feature/deseng661: Implemented new admin navigation design. #2565
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2565 +/- ##
==========================================
+ Coverage 75.99% 76.01% +0.01%
==========================================
Files 609 609
Lines 21924 21940 +16
Branches 1747 1752 +5
==========================================
+ Hits 16662 16678 +16
- Misses 4998 4999 +1
+ Partials 264 263 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| // If the route name doesn't match an expected page title, return the broken link icon | ||
| const possiblePageTitles: string[] = Object.keys(iconAssignments).filter((key) => key.includes(route.name)); | ||
| if (0 === possiblePageTitles.length) { | ||
| return <FontAwesomeIcon icon={faLinkSlash} style={fontAwesomeStyles} />; |
There was a problem hiding this comment.
nice touch with the broken link icon :)
| setOpen: (open: boolean) => void; | ||
| } | ||
|
|
||
| export interface IconAssignments { |
There was a problem hiding this comment.
This seems like a lot of trouble to go to... I'm thinking you should just extend the interface Route to include an icon?: IconDefinition which you can then render in your list mapping (or the broken link otherwise). Let me know if there'd be an issue with that
There was a problem hiding this comment.
Also, aren't these other strings redundant if you have [k: string]?
There was a problem hiding this comment.
I changed this based on your second comment, I didn't realize that I could just define the structure and not the keys.
| return <FontAwesomeIcon icon={iconAssignments[route.name]} style={fontAwesomeStyles} />; | ||
| }; | ||
|
|
||
| const CloseButton = () => { |
There was a problem hiding this comment.
Sonarcloud is suggesting (and I tend to agree) that this component definition should be extracted from the DrawerBox component and have its onClick behaviour passed in. Otherwise, just create it inline and use <When>
There was a problem hiding this comment.
No problem, I changed this so that the component is not within the DrawerBox.
| </List> | ||
| </Box> | ||
| ); | ||
| }; | ||
|
|
||
| const SideNav = ({ open, setOpen, isMediumScreen, drawerWidth = 280 }: SideNavProps) => { | ||
| const SideNav = ({ open, setOpen, isMediumScreen, drawerWidth = 300 }: SideNavProps) => { | ||
| if (!drawerWidth) return <></>; | ||
| return ( | ||
| <ThemeProvider theme={DarkTheme}> |
There was a problem hiding this comment.
The DarkTheme provider around this component no longer really makes sense and should be removed, since the menu has gone from a dark background to a light one. This will probably save you some unnecessary style overrides deeper in the component.
There was a problem hiding this comment.
Thanks, I removed it.
…disabled default button animations.
There was a problem hiding this comment.
Thank you for addressing my comments! If you could merge #2566 into this before it goes to main I'd appreciate it <3
| // Otherwise return the icon for the icon assignments key name that matches the route name. | ||
| return <FontAwesomeIcon icon={iconAssignments[route.name]} style={fontAwesomeStyles} />; | ||
| }; | ||
|
|
||
| return ( | ||
| <Box |
There was a problem hiding this comment.
Nice making this menu a list in HTML! Since this is a page region, assistive technologies can benefit from having it marked as such. The below code will make the outer element a <nav> and give it a label.
https://www.w3.org/WAI/tutorials/page-structure/regions/#navigation
| <Box | |
| <Box | |
| component="nav" | |
| aria-labelledby="Editor navigation" |
There was a problem hiding this comment.
Aria-labelledby is for passing in an element ID; you want aria-label instead
There was a problem hiding this comment.
Thanks @NatSquared that's my bad. Also I think the label itself could probably be improved to something like "Admin navigation menu"
* Patch for DESENG-661: Optimizations and Accessibility * Unify route item styles * Sonarcloud issues? Fixed
| width: '1.1rem', | ||
| }} | ||
| /> | ||
| <MetHeader4 |
There was a problem hiding this comment.
Folks using assistive technologies will expect headers to denote a section to follow. If you just used an <h4> to make the text large and prominent, then you should use a link tag and style it to be large and prominent instead.
Also I'd strongly recommend we update the openHelpPage function to open the link in the same window. Using new windows/tabs can confuse some users and are discouraged unless preserving the current context of the page is absolutely necessary: https://www.w3.org/WAI/WCAG21/Techniques/general/G200
There was a problem hiding this comment.
I'm just seeing a few places where we can improve the accessibility experience. Can we discuss before you proceed?
- Make the parent HTML tag of the menu a
navand label it - Change the Buttons in the menu to Links
- Don't use Heading tags in the nav menu unless they describe a section to follow. For large, prominent text, just use styling
|
Issue #: 🎟️ DESENG-661
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).