Navbar mobile refactored design #1568
Conversation
mertbagt
requested review from
sashamaryl,
nesanders,
mvictor55,
timblais,
alexjball and
Mephistic
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
LGTM
- 100% agree on pushing for Bootstrap's theming rather than fighting it - we can have a bigger discussion with design in a dev/design sync about standardizing on color palettes if need be.
- I vote to push back on the design here - we may actually need this, but this is a great case to confirm with design that we want to fight the natural flow of our UX library. Super weird that styled components doesn't accept the height (since we use that to set height in other components) - I'll keep an eye out for that going forward.
| const BlackCollapse = styled(() => { | ||
| return ( | ||
| <Navbar.Collapse id="basic-navbar-nav" className="bg-black mt-2 ps-4"> | ||
| {/* while MAPLE is trying to do away with inline styling, * |
There was a problem hiding this comment.
Yeah, sadly setting specific heights aren't covered by bootstrap. Weird that styled-components aren't respecting that - not worth digging into here, but thanks for the heads up to look out for this issue in the future.
That said, why do we need to set a specific height here? Shouldn't the menu just be the height of the links list, whichever set of links we're dealing with?
| </Navbar.Collapse> | ||
| ) | ||
| })` | ||
| .bg-black { |
There was a problem hiding this comment.
nit: Could this just use the bootstrap provided bg-dark and save some boilerplate? IMO we should probably start pushing designs to standardize on our bootstrap theme colors anyway.
| const toggleNav = () => setIsExpanded(!isExpanded) | ||
| const closeNav = () => setIsExpanded(false) | ||
| const toggleSite = () => { | ||
| if (isExpanded && whichMenu == "profile") { |
There was a problem hiding this comment.
nit: for these toggles, if we're setting menu to the same value in both branches, should we move the setMenu call outside the branch?
Summary
closes #1390
https://www.figma.com/file/IyRpfsVrU2pg3rysLauvQR/Navigation-V2?type=design&node-id=4356-14&mode=design
Checklist
Screenshots
Known issues
the dropdown menus are grey instead of black in the Figma. The coloring is pretty hardwired into Bootstrap's theming and is resistant to being overwritten. Likely, not worth dropping the themeing and doing the entire nav bar's coloration by hand.
The Figma wants the expanded Collapse element to fill up the whole screen below the navbar. For some reason, anytime I tried to set height with either Bootstrap or styled.components, the browser would just ignore that. I know we are trying to move away from inline styling but in this specific case, it's the only thing the browser seems to respect, so it's one-off exception, barring a better solution being found.
like the Desktop view, a General Search function needs to be developed and then can be added to both this and the desktop view
Steps to test/reproduce