(feat) O3-3774: Add implementation for the Nav Group #1116
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
|
Size Change: -430 kB (-6.99%) ✅ Total Size: 5.73 MB
ℹ️ View Unchanged
|
ibacher
requested changes
Aug 16, 2024
There was a problem hiding this comment.
I think this PR both extracts too much and too little. The only things we should need to move (AFAICT) are the generic-nav-group and maybe the generic-dashboard. I don't really see the value of or how anyone would even make use of the DashboardGroup stuff generically (it works in the chart, let's keep it there and simplify what this PR needs to do).
packages/framework/esm-styleguide/src/left-nav/CreateDashboardGroup.tsx
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/left-nav/generic-nav-group.component.tsx
Outdated
Show resolved
Hide resolved
ibacher
reviewed
Aug 19, 2024
|
So two other things need to happen for this PR to work:
|
|
@ibacher Most of what is in the generic-nav component is what is in the nav-group component unless if i am missing something |
|
By registering, do you mean creating a store and registering it in the store as they did in the common-lib? like this; const navGroupStore = createGlobalStore('global-nav-groups', { navGroups: [] });
export function registerNavGroup(slotName: string) {
const store = navGroupStore.getState();
navGroupStore.setState({ navGroups: [slotName, ...store.navGroups] });
} |
No. I mean actually registering the extension with the extension system, via the This is why I commented suggesting to rename the extension |
|
@ibacher anything we haven't covered in the PR, just want to know if there is more work or just the final review. |
ibacher
reviewed
Aug 29, 2024
|
@ibacher do we still want to move this into core or we close it |
ibacher
approved these changes
Sep 28, 2024
Samstar10
pushed a commit
to Samstar10/openmrs-esm-core
that referenced
this pull request
Oct 29, 2024
* Add nav group extension * Update docs * clean up * Remove unnecessary components * update docs * clean up * update t * define feature name for nav group * update docs * make group title translatable * update docs * tweak title translation and slot name * update api docs * register the global nav group extension * update docs * clean up * PR reviews * Update docs * Move nav group to primary-navigation-app --------- Co-authored-by: Ian <[email protected]>
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
Screenshots
I couldn't link the framework to the patient chart so I tested with the primary navigation app instead and below is a demo.
nav-group-demo.mov
Related Issue
Other