Fix Nav styles to match DS header #10823
Conversation
pettinarip
requested review from
samajammin,
wackerow,
corwintines,
minimalsm,
jmcook1186 and
nhsz
as code owners
src/components/Nav/index.tsx
Outdated
| variant="secondaryGhost" | ||
| px={1.5} |
There was a problem hiding this comment.
I'd like to have your 👀 on this PR @TylerAPfledderer, especially with the following changes:
1st change is in the Button component, where I had to remove the sx inner logic because otherwise, I wasn't able to override the px prop. Not sure exactly why (I think it is because the sx prop has more priority than the style props).
2nd change is a consequence of the 1st one. Since I removed the usage of useStyleConfig and sx from the Button component (and ButtonLink), I started to see the warning about the isSecondary flag. I ended up removing it and creating 2 new variants to cover the secondary cases. Not ideal I think, but it worked and is simple to understand.
Let me know what do you think about these changes or if you have any concerns. Also, feel free to test the rest of the changes as well and leave any comments :)
There was a problem hiding this comment.
To the first, I can see where the issue would happen, so this is definitely cleaner! 😄
As to the second, it is certainly a matter of preference. I guess here it is readability versus overkill? I'm good with this though! Indeed still easy to roll with.
There was a problem hiding this comment.
Thanks @TylerAPfledderer
This PR is a draft because I wanted to first check with you about it and see if you had a workaround to keep the same convention you built (with simpler variants + the isSecondary flag). Tbh I don't like having variants called "secondaryOutline" hahaha.
So, let me know if you prefer to hold on a bit here and see if you can add the necessary changes to make this work with the original convention you did.
There was a problem hiding this comment.
@pettinarip ok! We can hold here while I check it out 😁
There was a problem hiding this comment.
Awesome thanks!
LMK if you can add commits to this PR (I doubt it) or create a new PR off this branch and we can merge it back.
There was a problem hiding this comment.
When I try to clone this, it fails to create a branch. Only establishing the upstream (maybe) but stays on the dev branch with all the changes you have made staged
| isDarkTheme ? "Switch to Light Theme" : "Switch to Dark Theme" | ||
| } | ||
| icon={<Icon as={isDarkTheme ? MdWbSunny : MdBrightness2} />} | ||
| variant="icon" |
There was a problem hiding this comment.
Trying to remove as much as I can the icon variant to finally delete it from the theme.
| mobileNavProps, | ||
| } = useNav({ path }) | ||
| const searchModalDisclosure = useDisclosure() |
There was a problem hiding this comment.
Moved the search modal logic to the parent to avoid using refs which was causing me a headache.
| ) | ||
| ) | ||
|
|
||
| const Search = forwardRef<{}, "button">((_, ref) => { | ||
| const searchButtonRef = React.useRef<HTMLButtonElement>(null) | ||
| const { isOpen, onClose, onOpen } = useDisclosure() |
There was a problem hiding this comment.
Main change here was the removal of this line. It was moved to the parent component (the Nav).
✅ ethereum-org-website-dev deploy preview ready
|
src/components/Nav/index.tsx
Outdated
| <Button | ||
| aria-label={ | ||
| isDarkTheme | ||
| ? "Switch to Light Theme" | ||
| : "Switch to Dark Theme" | ||
| } | ||
| variant="secondaryGhost" | ||
| px={1.5} | ||
| onClick={toggleColorMode} | ||
| > | ||
| <Icon as={isDarkTheme ? MdWbSunny : MdBrightness2} /> | ||
| </Button> |
There was a problem hiding this comment.
Using IconButton also works here, and still simplified from the original
| <Button | |
| aria-label={ | |
| isDarkTheme | |
| ? "Switch to Light Theme" | |
| : "Switch to Dark Theme" | |
| } | |
| variant="secondaryGhost" | |
| px={1.5} | |
| onClick={toggleColorMode} | |
| > | |
| <Icon as={isDarkTheme ? MdWbSunny : MdBrightness2} /> | |
| </Button> | |
| <IconButton | |
| icon={isDarkTheme ? <MdWbSunny /> : <MdBrightness2 />} | |
| aria-label={ | |
| isDarkTheme | |
| ? "Switch to Light Theme" | |
| : "Switch to Dark Theme" | |
| } | |
| variant="secondaryGhost" | |
| px={1.5} | |
| onClick={toggleColorMode} | |
| /> |
There was a problem hiding this comment.
Regarding this point, I wasn't able to do that. I mean, the IconButton implementation was adding a different padding or margin (can't remember) to the inner icon, so that is why I ended up using the normal Button. Not sure if we should add something on the theme config to fix this.
There was a problem hiding this comment.
I'm not sure why it would be doing that. I'm not seeing that issue when I try to apply it. Maybe something to follow up with later once this PR gets pushed?
There was a problem hiding this comment.
Hmm probably I was just doing silly things and it works fine xD
Yea...lets leave it for now and we can tackle it later.
fix(Button): reintroduce `isSecondary` prop
Description
This PR fixes the styles issues reported in #10807 but also refactor the
Navcomponent to match the DS.TODO
iconvariantisSecondary) and refactor accordinglyRelated Issue
#9546
Fixes #10807