-
Notifications
You must be signed in to change notification settings - Fork 93
LF-4807 - Update main navigation styling #3894
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
Conversation
| import { getLanguageFromLocalStorage } from '../../../util/getLanguageFromLocalStorage'; | ||
|
|
||
| const MenuItem = forwardRef(({ history, onClick, path, children, className }, ref) => { | ||
| const isActive = matchPath(history.location.pathname, path); |
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.
inactiveListItem styles now applied to base style.
| styles.container, | ||
| isCompact && styles.compactContainer, | ||
| !isCompact && hasBeenExpanded && styles.expandedContainer, | ||
| styles.sideMenuContent, |
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.
Rename css styles -- "container" seemed to generic to understand while working on it. This component function is called sideMenuContent so I labeled it as such for understanding while working on PR.
| </ListItemButton> | ||
| {mainActions.map(({ icon, label, path, subMenu, key, badge }) => { | ||
| if (!subMenu) { | ||
| if (subMenu) { |
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.
Swapped the placement of the returns -- Rendering the exception is now done in the if statement instead of being the default.
| } | ||
|
|
||
| return ( | ||
| <React.Fragment key={key}> |
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.
Same - Swapped the placement of the returns -- Rendering the exception is now done in the if statement instead of being the default.
|
|
||
| return isMobile ? ( | ||
| <div className={styles.sideMenu}> | ||
| <div className={styles.drawer}> |
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.
CSS classname rename -- sideMenu was too generic to follow while updating -- this is the drawer version of the side menu only.
| padding: 0; | ||
| } | ||
|
|
||
| .listItem { |
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.
Nested under list now for specificity and shared color constants.
| background-color: var(--secondaryGreen50); | ||
|
|
||
| .subItemText { | ||
| .listItemText { |
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.
Nested under listItem now for specificity and shared color constants.
| &.subItem { | ||
| background-color: var(--secondaryGreen50); | ||
|
|
||
| .icon { |
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.
Nested under listItem now for specificity and shared color constants.
|
Thanks for catching the reversed hover-active on the original implementation of menu! Did you connect with Loïc by chance on the lack of blue on the mobile view? I'm not sure if that's intentional or an oversight (looks like those screens for mobile are still in their original form) -- do you know? The other thing I'm not sure if it is an original thing or an addition at this stage is the sub-menu styling on Figma here. It looks like it needs to have a little vertical bar on the left end? We might want to double-check that with Loïc as well. With the new CSS grouping (🙏) I think adding a specific style there should be pretty easy though! And not regarding the code at all, but I do want to check that we really want the active state blue to be SO blue -- it's so striking it makes it look like the primary colour of app (stronger than any green on page) and I kind of want to give that a double-check in dev-design 😅 Edit: I don't think any of that has to block merge though, right @Duncan-Brain ? We can add in a subsequent PR? Unless you think implementing blue on mobile or that little submenu bar would be incompatible with any of the code restructuring here? |
|
@kathyavini No I didn't connect with Loic on the hamburger menu blue color. I think the mobile menu was at least color differentiated so it seemed like a design choice. Forgot about the vertical bar, will double check those styles and update tomorrow! All you on the blue haha! |
|
Merged then! Will do follow up. |
|
@Duncan-Brain alright, if you love the Windows-level blue I won't say anything 🤐 (Well, unless Sayaka or Denis or someone backs me up on this once this is on beta 😂) |
Description
This PR adjusts the side menu styling according to figma.
Notes:
Jira link:
@kathyavini was there a PR for this? I couldn't find it.Sorry I found it: https://lite-farm.atlassian.net/browse/LF-4807Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
pnpm i18nto help with this)