(BREAKING) O3-3191: Move LeftNav into primary navigation app #978
Conversation
|
Size Change: -628 kB (-16.61%) 👏 Total Size: 3.15 MB
ℹ️ View Unchanged
|
| @@ -1,31 +1,10 @@ | |||
| /** @module @category UI */ | |||
| import React from 'react'; | |||
| import { ExtensionSlot, useStore } from '@openmrs/esm-react-utils'; | |||
| import { createGlobalStore } from '@openmrs/esm-state'; | |||
| import { ExtensionSlot, useStore, leftNavStore } from '@openmrs/esm-framework/src/internal'; | |||
There was a problem hiding this comment.
I've run into very esoteric problems with this. What happened in my case was that it seemed that Jest would come across this line, load the internal library, which loads the entire styleguide/index, which at some point loads some SVGs, and Jest sees the SVGs and crashes with syntax errors. Importing from the sub-libraries directly avoids that problem.
| const appMenuItems = useConnectedExtensions('app-menu-slot'); | ||
| const { slotName: leftNavSlot } = useStore(leftNavStore); | ||
| const leftNavItems = useConnectedExtensions(leftNavSlot); |
There was a problem hiding this comment.
Another pattern to consider is one I introduce in #979: using the featureName of the root component for the slot name. So like the feature name for patient chart is patient-chart. If we add featureName to ComponentContext then you can just get that context and make the slot name like left-nav-${featureName}-slot.
I have no opinion on whether that pattern is better or worse for this use case, I just want to put it out there that it's an option.
There was a problem hiding this comment.
Actually, I'm going to play around with that. Right now, the major problem with this PR is the need to manually call setLeftNav() and unsetLeftNav(), and this would give us a clean way of having the left nav just "magically" supported.
There was a problem hiding this comment.
I love that after like five years of extremely marginal existence featureName is suddenly becoming very useful
There was a problem hiding this comment.
@ibacher It would be great if this also supported some sort of default-left-nav-slot, which would render if and only if no specific left nav is set. This would be different from the existing global nav slot, which always renders. This way, implementations could basically have a left nave that is consistent across all routes in the application, which is then replaced for specific routes (eg. patient chart).
| @@ -42,7 +46,8 @@ const HeaderItems: React.FC = () => { | |||
| [], | |||
| ); | |||
|
|
|||
| const showHamburger = useMemo(() => !isDesktop(layout) && navMenuItems.length > 0, [navMenuItems.length, layout]); | |||
| const showLeftNav = useMemo(() => isDesktop(layout) && leftNavItems.length > 0, [leftNavItems.length, layout]); | |||
| const showHamburger = useMemo(() => !isDesktop(layout) && leftNavItems.length > 0, [leftNavItems.length, layout]); | |||
There was a problem hiding this comment.
| const showHamburger = useMemo(() => !isDesktop(layout) && leftNavItems.length > 0, [leftNavItems.length, layout]); | |
| const showHamburger = useMemo(() => !isDesktop(layout) && leftNavItems.length > 0, [leftNavItems.length, layout]); |
nit: showHamburgerMenu reads better here.
packages/apps/esm-primary-navigation-app/src/components/left-nav/index.tsx
Outdated
Show resolved
Hide resolved
|
So, a question on this: I was looking into the However, this has two disadvantages:
It's possible to work around these. For example, I thought we could address 1 by adding a "name" property for pages, with some sort of fallback for legacy code. 2 could be addressed by associating a "type" with each page, maybe "Main Screen" and "Component" or something, so that we could remove the hard-coding and only look for "Main Screen" components. But I'm also not sure if the added properties are worth it to solve what are likely corner-cases. Any opinions on this? |
|
What about using the overall route to determine what nav extension slot to render? Similar to how the activation function works generally for pages, but applied to slots / extensions? And if multiple are matched, then the most specific wins? Let's assume an extension slot base (suffix) of So, when the url starts with But if someone wanted a custom left nav for, say, service-queues, which would by default match up with the This would also inherently allow for defining a global, default slot by registering extensions for the root url at |
|
Yeah, definitely an interesting idea, @mseaton . It might require a little more infrastructure around how we relate the app state to the URL, but that might be a good thing. I kind of prefer that to a solution which misbehaves if there are multiple apps per page.
Can you expand a bit on that @ibacher ? I don't understand. |
Well, I was trying to do away with the need to call
So, I generally agree that using the URL makes sense. I don't totally love defining multiple possible slots for nested URLs. E.g., for Using just the first part of the URL might be tempting to avoid checking numerous possible slots when trying to figure out the left nav slot, but we'd have to figure out how we distinguish between the chart ( The idea of using the mounted apps was to basically be able to leverage apps |
|
Ohhhh right, of course, if the left nav is part of primary-navigation-app the feature name it receives would always be primary-navigation-app. Why do we want to move LeftNav into the primary navigation app, actually? One solution is to leave the left nav as it is, exported by the styleguide, rather than moving it into the primary navigation app. And then the LeftNav component would get the feature name of the app which has used it. To render the "default" left nav it could take an optional prop like |
ibacher
force-pushed
the
breaking/move-left-nav-to-primary-nav
branch
from
256d749
to
6a8246f
Compare
ibacher
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return uniq( | ||
| mountedApps.map((page) => { | ||
| let pageName = page; | ||
| pageName = pageName.replace(/^@[^\/]*\/(?:esm-)?(.+)-app-page-.*$/, '$1'); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
ibacher
force-pushed
the
breaking/move-left-nav-to-primary-nav
branch
from
dc641c5
to
14f6b59
Compare
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This extracts the LeftNav component from the styleguide into the primary navigation app, with the idea being that we have a single left nav that changes contents as the user browses.
This means that the API functions (now migrated to esm-state instead of esm-styleguide)
setLeftNav()andunsetLeftNav()are quite important. Essentially a page opts into using the LeftNav by callingsetLeftNav()to declare the slot. The LeftNav is only shown when there is at least one extension bound to the declared slot. So although the left nav is now a single component, it can contextually change which links are shown based on the extension slot name.NB This also means that the hamburger menu is only shown if there are any extensions mounted in the nav.
TODO: Figure out when it makes sense to fire an automated call to
unsetLeftNav()this probably needs to be done in the pagemount()call. I'll play with that.Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-3191
Other
PRs are coming for Home and Patient Chart. Patient Chart, in particular, will require some work to make this happen.
Also, if you test out this PR right now, you'll probably see no real changes (other than a weird rendering of an empty bar on the home page); it does appear to work, but the left nav items in Home and Chart are currently rendered over the nav bar.
Related PRs: openmrs/openmrs-contrib-json-schemas#10