Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Right-hand sidebar for staking flow #132
Right-hand sidebar for staking flow #132
Changes from all commits
c7dcd1f
f69a4a5
5155fc2
e61b042
540a59e
ea9aaf6
20edfd1
0ce1bd1
1edeff9
8763e4a
9ca695f
067b12a
bd81f8a
4a719ab
d4abdf1
8481695
6030fe2
dc581d4
6eb2029
86531de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is better to remove some of the props and make this component more flexible. And do something very similar to what was done in the threshold dashboard. I'm thinking of something like this:
In that case, we would also need to update the theme for the
Link
component. This is just a suggestion to improve this component. Let me know what you think, we could discuss it together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of flexibility? Prop
link
was removed. This component will be changed so on, because of the approach to creating chakra icons (instead chakra icons will use a common Image component).It will be changed if needed also here:
#141
#177
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my thoughts:
solid
icon can only be white.justifyContent
andvariant
. These props are already declared in theButtonProps
. You can easily change them using{...props}
in the component or set default values as I show above. But do we want to set a default value for the variant? It makes me wonder.isFullWidth
orlabel
is also unnecessary. Instead of alabel
, we can pass children.Link
, we can add more props. For examplehref
. It will also clearly show that we are using this button as a link.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prop
, so it can be overridden if needed, but by default, all buttons need to be aligned to the left.Link
component to handle externalhref
or handle it in the passed method onClick. We talked about this on Friday and we agreed as it is, so as not to go through the comments one more week.If needs arise, this component and others can be developed further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comment
We would probably want to change the name for the
header_height
. Of course, this isn't the scope of this PR but it can be a bit confusing at the moment. What I mean is that looking atheader_height
you start to wonder why it isn't in the sizes category. We'll have to improve this in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a task for this, we agreed that we do not want to mix different changes in one PR, so let's stick to this rule.