OCD-5292 | Navigation and Active State Handling#1518
Conversation
…link highlighting and hover effects [#OCD-5292]
…encies and updating link styles [#OCD-5292]
| color: palette.primary, | ||
| fontSize: '14px', | ||
| '&:hover': { | ||
| backgroundColor: `${palette.secondary} !important`, |
There was a problem hiding this comment.
Is this !important actually important?
| return () => window.removeEventListener('hashchange', handleHashChange); | ||
| }, []); | ||
|
|
||
| const isActive = href && currentHash === href; |
There was a problem hiding this comment.
What's the point of this? It's not a function, which I'd expect it to be if it's dependent on some state variable or variables. href must be defined, as per the propTypes definition, and this const? is only used in one place. It's just confusing. Couldn't the place where it's used just have the currentHash === href line inside the ${isActive... part?
| drawerItem: { | ||
| color: palette.greyDark, | ||
| '&:hover': { | ||
| backgroundColor: `${palette.secondary} !important`, |
There was a problem hiding this comment.
Is !important required? I'm having trouble checking on this. If it's truly important, that's fine, but that tag is such a strong indicator that it can easily override something else, so should be used only when absolutely required
| useEffect(() => { | ||
| const handleHashChange = () => setCurrentHash(window.location.hash); | ||
| window.addEventListener('hashchange', handleHashChange); | ||
| return () => window.removeEventListener('hashchange', handleHashChange); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
I've seen this construct three times in three files. If it was just once, that would be fine. If it was twice, that would suggest an option to raise it. Once it hits three times it should be in some sort of upper context level
2 participants
This pull request enhances the navigation components across the application by improving active state highlighting, refining menu item styling, and unifying navigation behavior for both desktop and mobile views. The changes ensure a more consistent user experience by visually indicating the current page, handling navigation events more robustly, and improving accessibility and analytics tracking.
Navigation and Active State Handling:
admin-menu-link-item.jsx,desktop-nav.jsx, andmobile-nav-drawer.jsxfor better user feedback. [1] [2] [3] [4] [5] [6]Styling and Visual Improvements:
Navigation Behavior and Analytics:
Code Quality and Imports:
Component Clean-up:
indicateOnHoverprop fromChplLinkusage in navigation components. [1] [2]These changes collectively improve navigation clarity, maintainability, and user experience throughout the app.