Navigation Component: Remove setActiveLevel child function arg #24704
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
psealock
added
the
[Feature] Navigation Component
psealock
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
psealock
changed the title
psealock
changed the title
|
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
joshuatf
reviewed
Aug 21, 2020
| @@ -91,7 +103,6 @@ function Example() { | |||
| onClick={ ( selected ) => | |||
| setActive( selected.id ) | |||
| } | |||
| setActiveLevel={ setActiveLevel } | |||
There was a problem hiding this comment.
One concern I have with removing this is that we limit much of the control over the state of this menu externally.
For example, imagine a WooCommerce helper page that says "Navigate to the Settings section in the menu." We can allow the click handler of the Settings link to navigate the side menu. Tutorial based components may also want to make use of this function.
I think this could also be done through props if you prefer (if ( activeLevelId !== prevActiveLevelid ) { setActiveLevel( activeLevelId ) }), but I'd like to not restrict control here too much.
There was a problem hiding this comment.
Just so I understand what you're suggesting: the feature you'd like to keep is the ability to externally control the navigation component without actually navigating.
I can see why that might be useful in some circumstances. What if we exposed an optional top level prop called activeLevel? That would accomplish the same result without having to expose a function. That brings its own challenges we'd need to document, but I think it would be more straightforward than using setActiveLevel function.
What do you think?
There was a problem hiding this comment.
Sure! That sounds like a good strategy to me. Do you want to do it here or should we tackle in a follow-up?
There was a problem hiding this comment.
Lets follow up, issue here: #24775
| @@ -39,13 +41,24 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| } | |||
| }, [] ); | |||
|
|
|||
| const NavigationBack = ( { children: backButtonChildren } ) => { | |||
| return ( | |||
There was a problem hiding this comment.
If we do keep this component instead of passing setActiveLevel maybe we should also include the logic for parentLevel here instead of in the story:
if ( ! parentLevel ) {
return null;
}There was a problem hiding this comment.
Thats a good addition (a897c42). And the implementation can also do logic like this, so I'll leave the check in the story.
{ parentLevel ? (
<NavigationBackButton>
<Icon icon={ arrowLeft } />
{ parentLevel.title }
</NavigationBackButton>
) : (
<Button>Do something else</Button>
) }| @@ -39,13 +41,24 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| } | |||
| }, [] ); | |||
|
|
|||
| const NavigationBack = ( { children: backButtonChildren } ) => { | |||
There was a problem hiding this comment.
Semantic nitpick:
Suggested change
| const NavigationBack = ( { children: backButtonChildren } ) => { | |
| const NavigationBackButton = ( { children: backButtonChildren } ) => { |
There was a problem hiding this comment.
Nice, fixed in 279bd0c
psealock
force-pushed
the
feature/navigation
branch
from
1c2111b to
aa9d063
Compare
psealock
force-pushed
the
fix/navigation-component-setActiveLevel
branch
from
a897c42 to
b4a02f6
Compare
joshuatf
approved these changes
Aug 25, 2020
psealock
added a commit
to psealock/gutenberg
that referenced
this pull request
Aug 26, 2020
…ress#24704) * remove setActiveLevel from public api * change to NavigationBackButton * return null for backButton if no parentLevel
psealock
added a commit
that referenced
this pull request
Aug 26, 2020
* Navigation Component: Remove setActiveLevel child function arg (#24704) * remove setActiveLevel from public api * change to NavigationBackButton * return null for backButton if no parentLevel * Navigation Component: Expose __experimentalNavigation component (#24706) * Expose __experimentalNavigation component * fix typo * expose menut and menu-item as well * Loop over navigation levels and wrap with animate * Use map and set to organize items and levels * Simplify level and item logic * Remove unnecessary key prop * Fix parent level assignment * Add back in key prop to force animation * Use useMemo to map item data Co-authored-by: Paul Sealock <[email protected]>
psealock
added a commit
that referenced
this pull request
Aug 31, 2020
* Navigation Component: Remove setActiveLevel child function arg (#24704) * remove setActiveLevel from public api * change to NavigationBackButton * return null for backButton if no parentLevel * Navigation Component: Expose __experimentalNavigation component (#24706) * Expose __experimentalNavigation component * fix typo * expose menut and menu-item as well * Loop over navigation levels and wrap with animate * Use map and set to organize items and levels * Simplify level and item logic * Remove unnecessary key prop * Fix parent level assignment * Add back in key prop to force animation * Use useMemo to map item data Co-authored-by: Paul Sealock <[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.
Fixes #24671
Description
Remove
setActiveLevelfromNavigationchild function arguments.setActiveLevelexposes the internal mechanism for keeping track of waterfall state, or in other words where the user is as they navigate the tree. The consuming app doesn't need to use it and exposing it only causes confusion.Its functionality is useful for the "Back" button but allowing the consuming app to compose the contents as they choose requires its use. This PR seeks a middle ground by passing down a component called
NavigationBackas a child function argument.Its use now encapsulates
setActiveLevelwhile allowing maximum composition.How has this been tested?
npm run storybook:devto see the Navigation component function.Screenshots
Checklist: