(feat) Create a global app history utility #877
Merged
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
… directly from esm-config
brandones
commented
Jan 9, 2024
brandones
commented
Jan 9, 2024
| @@ -0,0 +1,35 @@ | |||
| /** @module @category Breadcrumb */ | |||
There was a problem hiding this comment.
Copy-pasted from esm-breadcrumbs
|
Size Change: -655 kB (-19%) 👏 Total Size: 2.84 MB
ℹ️ View Unchanged
|
ibacher
reviewed
Jan 9, 2024
There was a problem hiding this comment.
Thanks @brandones! A couple of really minor points, but this looks great!
| export * from './types'; | ||
|
|
||
| import { registerBreadcrumb as rb, registerBreadcrumbs as rbs, getBreadcrumbs as gb } from './db'; |
There was a problem hiding this comment.
Honestly, I'd be fine with just dropping esm-breadcrumbs altogether. Anything not in the framework that depends directly on the sub-components is, in my view, relying on implementation details. We've pretty freely moved functions around before when it made sense to and I don't think this should be different (even the app shell loads these from the framework).
There was a problem hiding this comment.
I like that philosophy. Will clean this up.
Comment on lines
25
to
32
| initializeHistory(); | ||
|
|
||
| window.addEventListener('single-spa:routing-event', (evt) => { | ||
| const history = getHistory(); | ||
| if (history[history.length - 1] !== window.location.href) { | ||
| addToHistory(window.location.href); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Would it make sense to migrate this into the startup sequence in the app-shell? I kind of like the clarity of having a single piece of code that tells us everything being done at startup.
There was a problem hiding this comment.
Agreed, that's a nice idea, will do.
ibacher
approved these changes
Jan 11, 2024
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
I got into this in order to solve https://openmrs.atlassian.net/browse/O3-1594 . The trouble is that there is at present no good way for the close button to know where to go back to.
document.referreris often not what we want,window.location.back()will go back to the previous dashboard within the patient chart, and hacks involving localStorage are unpleasant.This creates a global app history that is preserved across the session. This way, the close button can look backwards through the history in order to determine where it should send the user back to. This will also allow us to make the breadcrumb actually correctly reflect where the user came from—right now the top level breadcrumb in the chart is always "home."
I created a new library to house this, called
esm-navigation, which I think can serve as a more natural home for some things. I moved the navigation utilities out ofesm-config(kind of a strange place for them) and into this new library, along with the whole ofesm-breadcrumbs.@openmrs/esm-breadcrumbsis now deprecated. We should delete it in the next core major version.I tried really hard to deprecate direct imports of
navigate,interpolateString, andinterpolateUrlfrom esm-config. Unfortunately it made things with tests and mocks and types very complex, and I wasn't able to get the whole thing working. So I've simply removed these fromesm-config. This is a breaking change for anyone who was doingimport { navigate } from '@openmrs/esm-config'(or likewise with the other functions). I doubt anyone was doing that though, which is why I have not attached the BREAKING label.Only vaguely related,
navigateis now a little bit smarter and will do a SPA navigation if possible even if the caller has included the origin in the URL. Previously,navigate({ to: 'https://dev3.openmrs.org/openmrs/spa/home' })would do alocation.assign.Screenshots
Will include screenshots in the patient chart PR.
Related Issue
Related to https://openmrs.atlassian.net/browse/O3-1594
Other