(feat) O3-4425 - add ability to define collapsed left nav #1279
Conversation
This file contains 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
chibongho
commented
Feb 3, 2025
| * nav when the hamburger menu is clicked on in tablet mode. See side-menu-panel.component.tsx | ||
| * | ||
| * Use of this component by anything other than <SideMenuPanel> (where isChildOfHeader == false) | ||
| * is deprecated; it simply renders nothing. |
There was a problem hiding this comment.
This is a bit hacky. If we feel comfortable with this, we can opt for more aggressive breaking change where we stop exporting <LeftNavMenu> in @openmrs/esm-framework, and force other ESMs to remove it.
|
Size Change: -152 kB (-2.4%) Total Size: 6.2 MB
ℹ️ View Unchanged
|
chibongho
changed the title
|
@ibacher @denniskigen @samuelmale @vasharma05 This touches core code, so I'm hoping to get feedback from outside PIH. Please review when you have a chance. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This PR adds the ability for apps to define a left nav that's always collapsed into the top nav hamburger menu.
This PR also includes refactoring to remove the need to render
<LeftNavMenu>within the home or patient-chart app. Currently, the home / patient-chart app needs to render<LeftNavMenu>in its root component in order for the left nav to show in desktop mode. The top nav, however, also has a<SideMenuPanel>, which renders the menu when in tablet mode and the hamburger button is toggled on. (Incidentally,<SideMenuPanel>actually uses<LeftNavMenu>as child component)Screenshot:
<LeftNavMenu>rendered by home app:Screenshot:
<SideMenuPanel>rendered by primary-navigation-app:The
<LeftNavMenu>is a carbon's<SideNav>component, which is aposition: fixedelement. This means we shouldn't really need the home or patient chart app to render it as part of its component tree. The refactoring makes it so the that the<SideMenuPanel>renders both:Screenshots
Video of the home app with this change. No noticeable difference from before. (Not part of this PR, but in the video capture,
<LeftNavMenu>is removed from the home app.)https://github.com/user-attachments/assets/f89bae32-5b20-4270-aa44-8874fff62cf4
Video of the dispensing app with this change, The dispensing app added the following to define the left nav to be always collapsed:
OpenMRS.Mozilla.Firefox.2025-02-03.16-48-33.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-4425
Other
I attempted to refactor
<LeftNavMenu>hoping this will give us a cleaner way to define a collapsed left nav. While this change removes the need to render<LeftNavMenu>in home / patient-chart, it fails to completely abstract the left nav logic away from those apps. In particular, the home dashboard actually needs to maintain amargin-leftin order to be rendered to the right of the left nav in desktop mode. (That margin is set to 0 in tablet mode).I'm not sure if there is a way around it as the left nav isThis is probably doable with a more invasive change by overriding theposition: fixed.position: fixedin the carbon<SideNav>and adding atop-nav-containerdiv inindex.ejs(so we can render the top nav there viacreatePortal()).