Components: Add support for named arguments in the navigator components#47827
Merged
Components: Add support for named arguments in the navigator components#47827
Conversation
youknowriad
added
[Type] Enhancement
|
Size Change: +2.93 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 23fb08c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4131841112
|
youknowriad
commented
Feb 7, 2023
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
youknowriad
commented
Feb 7, 2023
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
youknowriad
commented
Feb 7, 2023
Contributor
Author
There was a problem hiding this comment.
Noticed that alignment="left" doesn't work as expected in VStack. It seems useFlex forces alignItems to be "normal" regardless of the value provided by the useHStack hook (for the VStack case). I think that's probably a bug in useFlex that I'll explore separately.
Contributor
There was a problem hiding this comment.
Thank you for spotting. Let us know if you have any further findings!
youknowriad
commented
Feb 7, 2023
ciampo
reviewed
Feb 7, 2023
Contributor
There was a problem hiding this comment.
Thank you for working on this, Riad!
A few months ago I had spec'd out a tentative plan of action to introduce this feature to Navigator (#45913) , and it's great to see that the approaches are very much aligned (if not practically the same).
Apart from the inline comments, could you also add an entry to the CHANGELOG ? It looks like this PR could go under the Enhancements section.
Thank you!
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
youknowriad
force-pushed
the
add/support-named-args
branch
from
439a54e to
3e51946
Compare
Contributor
Author
|
@ciampo I think I've addressed all the review comments here. |
ciampo
reviewed
Feb 8, 2023
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Looking good!
Apart from pending inline comments, could we also update the components READMEs?
I think we'd mostly need to add the match object documented in the useNavigator docs
Thank you!
Contributor
Author
|
@ciampo Ok this one should be ready now. |
ntsekouras
pushed a commit
that referenced
this pull request
Feb 9, 2023
…ts (#47827) * Components: Add support for named arguments in the navigator components * Refactor to use useMemo * Improve the types * Switch to React's useId * Refactor how we store the focus target * Add unit tests * Add changelog entry * Rename the Screen type * Improve the types of the params argument * Add unit test * Early return * Add documentation
Contributor
|
Cherry-picked this PR to the wp/6.2 branch. |
ciampo
added
the
has dev note
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What?
Closes #45913
This PR adds supports for named arguments to the Navigator components. In other words, we now support paths with arguments like:
/product/:productId.Why?
In different places we rely on a pattern like this
This loop is actually not necessary if we had support for named arguments. So what I'm proposing in this PR is to replace the above code with `
An example has been added to the storybook.
In #47777 we don't have the list of all available templates (and we don't want to fetch all of them) so I was forced to separate that "argument" outside of the path (I've used a query arg in location) In order to solve this issue. Basically even the mapping above was not an option.
How?
path-to-regexppatch (very widely used in the JS community) to parse patterns like that.useNavigator. We now have amatchkey that stores the "id" of the active screen/route and theparamskeys with the parsed arguments from the path.Testing Instructions
1- Run
npm run storybook:dev2- Click the "open product 1" button in the "Navigator" story.
✍️ Dev Note
The dev note from #47883 includes changes from this PR too.