-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor: Support light mode in nav #2522
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action π€ This PR introduced no changes to the javascript bundle π |
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.
LGTM!
/* @nest html[data-theme='light'] & { | ||
&:active { | ||
background-color: var(--token-color-foreground-primary); | ||
} | ||
} | ||
} */ | ||
|
||
@nest html[data-theme='dark'] & { | ||
/* @nest html[data-theme='dark'] & { | ||
color: var(--token-color-foreground-strong); | ||
|
||
&:hover, &[aria-expanded='true'] { | ||
border-color: var(--token-color-border-strong); | ||
} | ||
} | ||
} */ |
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.
Why do we need to comment this code out?
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.
Oh, I was testing whether we needed that code, and left that in before review π I'll double-check what's needed from it and remove in the next commit if I can (I should be able to) π
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.
@nandereck Removed the comments as of this commit (they were safe to remove):
eaf25cf
π Relevant links
ποΈ What
This PR refactors the nav and relevant children components to support light mode.
π€· Why
This comes after a design team recommendation while working on other changes to the nav.
π οΈ How
My approach here was to leverage our existing tokens' ability to change based on the site theme, and use those tokens instead of hardcoded values in key areas. In some instances (e.g.: the
.siteLogo
SVG on the homepage and the logo on product pages), I addedpath
fill overrides to change the SVGs' color when the theme changes.πΈ Design Screenshots
There is no established spec for light mode at present; the preview has been approved by design.
π§ͺ Testing
π Anything else?
This work does change which text colors show up when switching to dark mode, but now the colors should be more aligned with our general design system.