OCD-4307 React Navigation Rehaul #240
Merged
andlar merged 39 commits intoandlar:OCD-4307from Apr 8, 2026
Merged
Conversation
- Created ChplAdminMenu component for administrative functionalities. - Integrated ChplAdminMenu into ChplToggle for logged-in users. - Updated styles in signin component for improved UI. - Enhanced navigation bar with new logo and updated styles. - Refactored toggle logic for better state management in navigation. - Added new SVG logo asset for branding. [#OCD-4307-UI]
…prove toggle functionality [#OCD-4307-UI]
…ality [#OCD-4307-UI]
…g and layout [#OCD-4307-UI]
…e forwardRef and improve accessibility [#OCD-4307-UI]
…onent in ChplAnnouncementsFab and change username to not be caps [#OCD-4307-UI]
… improve ChplNavigationTop with shimmer effect and close handlers [#OCD-4307-UI]
…improve state management [#OCD-4307-UI]
…improve state management in ChplToggle [#OCD-4307-UI]
andlar
reviewed
Feb 23, 2026
| display: 'flex', | ||
| flexDirection: 'row', | ||
| }, | ||
| wordWrap: { |
Owner
There was a problem hiding this comment.
This feels like it should be in the utilStyles file
Comment on lines
-114
to
+121
| function ChplCmsDisplay() { | ||
| const ChplCmsDisplay = React.forwardRef((props, ref) => { |
| marginBottom: '8px', | ||
| display: 'flex', | ||
| }, | ||
| wordWrap: { |
Owner
There was a problem hiding this comment.
It's used twice, the same way; that should be consolidated
Comment on lines
+77
to
+79
| if (onClose) { | ||
| onClose(); | ||
| } |
Owner
There was a problem hiding this comment.
The onClose function defaults to existing, with a "don't do anything" function, so having a check to see if it exists is confusing
| const handleLogout = () => { | ||
| authService.logout(); | ||
| onDispatch({ action: 'logout' }); | ||
| if (onClose) { |
| <AppBar position="static"> | ||
| <Toolbar> | ||
| <AppBar position="fixed" className={classes.appBar}> | ||
| <Toolbar style={{ backgroundColor: '#001439', display: 'flex', justifyContent: 'space-between' }}> |
Owner
There was a problem hiding this comment.
Is this color something in our palette? If not, should it be?
| <Toolbar> | ||
| <AppBar position="fixed" className={classes.appBar}> | ||
| <Toolbar style={{ backgroundColor: '#001439', display: 'flex', justifyContent: 'space-between' }}> | ||
| <Box ahref="#/search" display={"flex"} alignItems="center" onClick={home} style={{ cursor: 'pointer' }}> |
Owner
There was a problem hiding this comment.
What's ahref?
Why is display={"flex"} not display="flex"? Or could the display value be in the style tag at the end?
| <div className={classes.shimmer} /> | ||
| </div> | ||
| </Box> | ||
| <Box display={"flex"} alignItems="center"> |
Owner
There was a problem hiding this comment.
This seems weird too. Why is one in an interpreted JSX thing and the other just a pure string?
| anchorEl={anchorEl} | ||
| open={!!anchorEl && showCmsWidget} | ||
| anchorEl={cmsAnchorEl} | ||
| open={Boolean(cmsAnchorEl)} |
Owner
There was a problem hiding this comment.
I don't like the Boolean(element) convention as opposed to !!element, as it takes more time to type, and is technically slightly slower. That said, according to a quick Google search, the Boolean version is better for clean code and readability. I'm still leaning towards the !! version, as that's what we have everywhere else in this codebase, so unless you'd like to make the case that we convert those (as we can) I'd prefer we stick with the convention that's already established
| <Button | ||
| onClick={toggleCompareWidget} | ||
| className={classes.whiteButton} | ||
| color='inherit' |
[#OCD-4307-UI] Move duplicate wordWrap style definition to util.js for reuse across components
[#OCD-4307-UI] Add navBackground color to palette.js and update navigation-bottom.jsx to use it instead of hardcoded value for consistency
[#OCD-4307-UI] Remove duplicate wordWrap style definitions from cms-display and compare-display components, now using shared utilStyles
[#OCD-4307-UI] - Convert navigation items to use ChplLink components for consistency - Remove unnecessary onClose conditional checks (has default function) - Replace API Keys menu item with URL Checker - Add proper router props for client-side navigation - Remove unused navigateTo function and $state service
[#OCD-4307-UI] - Add test environment warning banner for non-production environments - Add arrow indicators to dropdown menus pointing to parent buttons - Fix findDOMNode deprecation warnings by using refs instead of event.currentTarget - Use palette.navBackground color for consistency - Adjust AppBar position when banner is visible
…re and mobile support - Added ChplDesktopNav for desktop navigation with CMS ID Creator and Compare Products features. - Introduced ChplMobileNavDrawer for mobile navigation, including collapsible sections for CMS ID Creator, Compare Products, Resources, and Shortcuts. - Updated navigation-top to integrate new desktop and mobile navigation components, enhancing responsiveness. [#OCD-4307-UI]
…iveness [#OCD-4307-UI]
…ple components [#OCD-4307-UI]
[#OCD-4307-UI]
Added ChplEnvironmentBanner to show warnings for non-production environments. Updated ChplMobileNavDrawer to use getResourceItems and shortcutItems for dynamic menus. Integrated the environment banner into navigation-top. Removed deprecated HTML from navigation-top.html and simplified navigation-top.jsx. [#OCD-4307-UI]
…e and mobile support [#OCD-4307-UI]
…and compare displays [#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
[#OCD-4307-UI]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors several UI components to improve consistency, accessibility, and maintainability. The main changes include migrating custom announcement and widget panels to Material-UI components, improving styling and layout, and updating the display logic for empty states. PropTypes and React forwardRef are also introduced for better type safety and composability.
Announcements panel modernization:
IconButton,Badge,Menu, andCardHeadercomponents for improved accessibility and visual consistency. Custom click-outside logic and manual styling were removed in favor of Material-UI's built-in features. (src/app/components/announcements/announcements-fab.jsx) [1] [2] [3]CMS widget improvements:
ChplCmsDisplayto useCardContentfor empty state, added improved layout and guidance text, and migrated to React.forwardRef for better composability. Added PropTypes toProgressBarfor type safety. (src/app/components/cms-widget/cms-display.jsx) [1] [2] [3] [4] [5] [6] [7] [8]Compare widget improvements:
ChplCompareDisplayto useCardContentfor empty state and migrated to React.forwardRef for composability. Improved styling and layout for consistency with other widgets. (src/app/components/compare-widget/compare-display.jsx) [1] [2] [3] [4]