-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add parent navigation support for the navigator component #47883
Conversation
Size Change: +232 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Thank you for working on this so quickly! For now, I focused my efforts on reviewing #47827. I should be able to take a look at this PR tomorrow. |
2855ee9
to
f5ac3ad
Compare
I've rebased this, it's still missing docs and changelog entry, but let's discuss implementation. |
}, [ screens ] ); | ||
useEffect( () => { | ||
currentLocationHistory.current = locationHistory; | ||
}, [ locationHistory ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to do this kind of useEffect. This is basically the perfect use case for the React's proposed useEvent. I'm looking forward to using it.
Flaky tests detected in 92d7520. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4146283866
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you once more for working on this! I appreciate the thoughtfulness and the research of collaboration in the approach!
I gave a look at both Storybook and Global Styles, and the component seems to behave as expected 🎉
Out of curiosity, I checked current usages of Navigator outside of Gutenberg, and there's a bunch of plugins using it.
As discussed previously, the approach proposed in this PR seems the most reasonable in order to improve the component to fulfill product needs, without introducing breaking changes.
We'll need to make sure we update docs with clear explanations on how to use this component — mainly:
- using it chronologically (ie. using
goBack
) or hierarchically (goToParent
) - if using
goToParent
,path
needs to have a URL-like format, starting with/
and using/
to separate each path segment.
Finally, I hope I'll be able to merge #47910 soon (currently experiencing CI issues), in order to have the additional confidence boost given by extra unit tests
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen/index.js
Outdated
Show resolved
Hide resolved
7498aec
to
f7b2918
Compare
cb92bcf
to
d350e54
Compare
I actually dove deeper into these results and it was interesting to note that all of them without exception use hierarchical paths. In other words, we could potentially decide to just replace goBack with goToParent and have no breakage. |
This PR should be ready for another round of review. |
packages/components/src/navigator/navigator-to-parent-button/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-to-parent-button/component.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should be ready for another round of review.
Tested in both Storybook and Site editor, and everything works as expected.
Apart from the inline comments, before merging we should also:
- Update the
navigator
object docs
Suggested changes
diff --git a/packages/components/src/navigator/navigator-provider/README.md b/packages/components/src/navigator/navigator-provider/README.md
index 3afd84b559..47c2af4b9d 100644
--- a/packages/components/src/navigator/navigator-provider/README.md
+++ b/packages/components/src/navigator/navigator-provider/README.md
@@ -34,13 +34,17 @@ const MyNavigation = () => (
</NavigatorProvider>
);
```
+
**Important note**
-Parent/child navigation only works if the path you define are hierarchical. For example:
- - `/` is the root of all paths.
- - `/parent/child` is a child of `/parent`.
- - `/parent/child/grand-child` is a child of `/parent/child`.
- - `/parent/:param` is a child of `/parent` as well.
+Parent/child navigation only works if the path you define are hierarchical, following a URL-like scheme where each path segment is separated by the `/` character.
+
+For example:
+
+- `/` is the root of all paths. There should always be a screen with `path="/"`.
+- `/parent/child` is a child of `/parent`.
+- `/parent/child/grand-child` is a child of `/parent/child`.
+- `/parent/:param` is a child of `/parent` as well.
## Props
@@ -65,6 +69,15 @@ The `goTo` function allows navigating to a given path. The second argument can a
The available options are:
- `focusTargetSelector`: `string`. An optional property used to specify the CSS selector used to restore focus on the matching element when navigating back.
+- `isBack`: `boolean`. An optional property used to specify whether the navigation should be considered as backwards (thus enabling focus restoration when possible, and causing the animation to be backwards too)
+
+### `goToParent`: `() => void;`
+
+The `goToParent` function allows navigating to the parent screen.
+
+Parent/child navigation only works if the path you define are hierarchical (see note above).
+
+When a match is not found, the function will try to recursively navigate the path hierarchy until a matching screen (or the root `/`) are found.
### `goBack`: `() => void`
- Ideally, add tests to
packages/components/src/navigator/test/index.tsx
using theNavigatorToParentButton
component
I actually dove deeper into these results and it was interesting to note that all of them without exception use hierarchical paths. In other words, we could potentially decide to just replace goBack with goToParent and have no breakage.
That's an interesting reflection — it means that, until now, all consumers of Navigator
are basically using it to browse a tree-like structure of screens by transition from parent to child, or from child to parent.
I still think that offering chronological goBack
could prove useful for a consumer that doesn't necessarily navigate screens from parent to child (or from child to parent).
For example, imagine these scenarios
- we show the top menu (typography, blocks, ...) in a navigation bar that is always visible in every screen. A user could use that navigation bar "jump" across screens.
- a user is editing typography settings for a specific block. We could show a "go to global typography settings" button that takes them to the global typography button
In both scenarios, a "go to parent" button (i.e. go to home) and a "back" button (ie., back to whatever screen was visited previously) would both be useful for different reasons.
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
I think I've addressed all the feedback and applied all suggestions @ciampo That said, CI and types started failing randomly, not sure exactly what I did that triggered that? Maybe your eagle eye can spot anything here? |
Ok the type failures were due to the import of the button component in the test. It should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two more minor comments, but we can go ahead and merge once they are solved
🚀
Thank you for the collaboration on this one! Navigator
feels in a much better place.
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Cherry-picked this PR to the wp/6.2 branch. |
Builds on top of #47827 and addresses an issue raised in #47777
What and Why?
Parent or "back" navigation doesn't work as we expect it to work in Global Styles and Site editor. The reason is that
goBack
behaves like a browser history, it shows the previous screen that was visible. Sometimes navigation happens programmatically and sometimes the initial screen rendered is not the top level ones (like after reloading the page). For these situations, goBack doesn't work for us. What we really want is a child/parent relationship where the "back" button always goes back to the parent screen.This PR adds support for that to the Navigator components.
How?
There are two main ideas here. A user can define a "tree" of screens by relying on the paths:
/
is the root screen/something
is a child of root/:argument
is also a child of root/something/grandchild
is a child of/something
if defined, otherwise it fallback to/
Basically to define the hierarchy you can split a path on the
/
character and move up until you find a screen that available.The second idea of this PR is that
goToParent
is equivalent togoToBack
and should restore the focus in the same way if the path is the previous in location history.Testing Instructions
1- You can open the "nested navigator" story in storybook and check the behavior.
✍️ Dev Note
The
Navigator
component from the@wordpress/components
package has been enhanced with two additional features:/product/:productId
);goToParent
function and theNavigatorToParentButton
component.These two features assume that
NavigatorScreen
s are assigned paths that are hierarchical and follow a URL-like scheme, where each path segment is separated by the/
character.