Right-hand sidebar for staking flow #132
Conversation
ioay
force-pushed
the
right-hand-sidebar-staking-flow
branch
from
03e7ca7 to
f69a4a5
Compare
| variant="outline" | ||
| justifyContent="flex-start" | ||
| leftIcon={<Icon as={ArrowUpRight} boxSize={6} color="brand.400" />} | ||
| onClick={openDocsDrawer} |
There was a problem hiding this comment.
Just flagging - we probably should think about how to pass the section name to the docs drawer to open the correct docs there.
There was a problem hiding this comment.
Yep, we'll deal with it when the docs're ready.
|
@kkosiorowska (#132 (comment)): We should keep all assets (like fonts/images, maybe also icons) in one |
dapp/src/theme/Sidebar.ts
Outdated
| @@ -18,6 +18,7 @@ const baseStyleSidebarContainer = defineStyle({ | |||
| const baseStyleSidebar = defineStyle({ | |||
| p: 4, | |||
| height: "100%", | |||
| w: "80", | |||
There was a problem hiding this comment.
It seems to me that we shouldn't declare widths here. In the Sidebar component, we set it again. I would suggest moving it to the component and setting it for the sidebar element. Something like this:
<Box __css={styles.sidebar} w={80}>
There was a problem hiding this comment.
It seems to me that we shouldn't declare widths here. In the Sidebar component, we set it again. I would suggest moving it to the component and setting it for the sidebar element. Something like this:
<Box __css={styles.sidebar} w={80}>
It has to be here for 3 reasons:
- it is not setting again, because you point Box with
sidebarContainerclass, this style is related to another Box withsidebarclass. - I don't see the reason to keep an additional style prop if we have already defined custom sidebar.
- we have to have defined fixed width, beacse in other case content inside this container while extending/collapsing sidebar content becomes floating too much, which looks very bad and we don't want this behavior.
There was a problem hiding this comment.
If we define the width to fixed 80 value here and in the mentioned Sidebar component
We need to maintain the value in two places. Can we find a way to improve it?
There was a problem hiding this comment.
If we define the width to fixed
80value here and in the mentioned Sidebar componentWe need to maintain the value in two places. Can we find a way to improve it?
Sure, I'll do that.
There was a problem hiding this comment.
I understand why we want to set this value. However, I still have doubts that it should be set in a component theme. I have doubts because what if we want to use this theme once again with a different width? This isn't likely to happen. But it makes me wonder.
There was a problem hiding this comment.
At the moment, looking at the design, there are no such requirements - If there is such a need, we will change it.
| variant="outline" | ||
| justifyContent="flex-start" | ||
| leftIcon={<Icon as={ArrowUpRight} boxSize={6} color="brand.400" />} | ||
| onClick={openDocsDrawer} | ||
| width="100%" | ||
| bg="gold.100" | ||
| borderColor="white" | ||
| borderStyle="solid" |
There was a problem hiding this comment.
I think we can also use the colorScheme property here because this button looks like a card variant. Only the background and border color have been changed. Check how it was done here.
There was a problem hiding this comment.
I added a sidebar variant to the button style guide. Already existing card variants have too many differences and expanding it will make a mess.
Sure, but I think we should not mix the changes with each other. Based on the directory structure, the space for the images was in the static folder. Of course, we can improve the structure and move them to the assets folder. But at this point in my opinion mixing it up causes a bit of a mess. Because some things have already been moved and some have not (for example icons). Doing it in a separate PR would be more clear. |
Already done in #142 |
… sidebar width, updated chakra variants for button & card
| @@ -3,4 +3,7 @@ export const semanticTokens = { | |||
| header_height: 24, | |||
There was a problem hiding this comment.
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 at header_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.
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.
| onClick: () => void | ||
| } | ||
|
|
||
| export default function ButtonLink({ |
There was a problem hiding this comment.
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:
import React from "react"
import { Button, ButtonProps, Icon, Link, LinkProps } from "@chakra-ui/react"
import { ArrowUpRight } from "#/static/icons"
type ButtonLinkProps = ButtonProps &
LinkProps & {
icon?: typeof Icon
iconColor?: string
}
export default function ButtonLink({
icon = ArrowUpRight,
iconColor = "brand.400",
...props
}: ButtonLinkProps) {
return (
<Button
as={Link}
w="100%"
justifyContent="start"
leftIcon={<Icon as={icon} boxSize={4} color={iconColor} />}
{...props}
/>
)
}
<ButtonLink variant="solid" onClick={openDocsDrawer} iconColor="white">
Docs
</ButtonLink>
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.
What kind of flexibility
Some of my thoughts:
- Currently, for the variant
solidicon can only be white. - I don't see the point of declaring in props:
justifyContentandvariant. 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. - In my opinion
isFullWidthorlabelis also unnecessary. Instead of alabel, we can pass children. - Also, Using this button as a
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.
- As I mentioned above - chakra icon will be changed to svg, so it's going to change. On Friday we agreed that the icon will be in orange/white color depending on the solid/outline variant. Let's leave it as it is.
- justifyContent is a
prop, so it can be overridden if needed, but by default, all buttons need to be aligned to the left. - ok, I'll change
- Let's not mix it up with link functionality now, if it will be needed we can extend this file or create a
Linkcomponent to handle externalhrefor 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.
I think we are ready to merge. 🚢 So let's not block that PR any longer. Some important notes:
- The
ButtonLinkwill be extended with the functionality of a link as it was done in the threshold dashboard. Created issue Extend buttonLink to handle href link #180 - The handling of the use of icons/images in the dapp will change. So the
ButtonLinkwill also be updated under this scope later. - At the moment, for the variant solid icon is always white for
ButtonLink. Let's update this when needed or we all agree on the change.
Closes: #109