Navigator: simplify backwards navigation APIs#63317
Conversation
ciampo
force-pushed
the
feat/navigator-simplify-apis
branch
2 times, most recently
from
f9155ed to
1a6ecf2
Compare
ciampo
added
[Type] Code Quality
…r NavigatorGoBackButton
ciampo
force-pushed
the
feat/navigator-simplify-apis
branch
from
05b5131 to
7650c06
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| raw: '/ "\'><=invalid_path', | ||
| escaped: "/ "'><=invalid_path", |
There was a problem hiding this comment.
Because of "going back" always behaving like "go to parent", all paths need to start with / in order to work well with the component's pattern matching logic
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This looks good to me 👍
Left a few minor suggestions.
I appreciate the simplification here. Makes the API clearer and more predictable, and the consumers better aware that they should prepare their navigation structure in a more hierarchical one-to-one way.
| @@ -4,6 +4,8 @@ | |||
| This feature is still experimental. “Experimental” means this is an early implementation subject to drastic and breaking changes. | |||
There was a problem hiding this comment.
Sounds like we can remove the experimental callout now.
There was a problem hiding this comment.
I was planning on removing those experimental callouts and Storybook tags once we also export the component without the experimental prefix (in a follow-up PR)
| deprecated( '`goToParent` prop in wp.components.NavigatorBackButton', { | ||
| since: '6.7', | ||
| alternative: | ||
| '"back" navigations are always treated as going to the parent screen', |
There was a problem hiding this comment.
Should we just suggest goBack()? For someone attempting to use goToParent, it's not immediately clear in their IDE that goBack is the recommended alternative.
There was a problem hiding this comment.
The NavigatorBackButton doesn't have a goBack prop, since "going back" was already the default behavior. The goToParent prop was changing that behaviour, but now that the default behaviour of "going back" is the same as "going to parent", that prop basically became a no-op and can just be omitted.
There was a problem hiding this comment.
Actually thanks to your comment, I realised that this deprecation was not necessary because that was a code path that could not be hit anymore. I just removed the code instead: da79e35
This can be done because its usage was actually only internal to the component
ciampo
force-pushed
the
feat/navigator-simplify-apis
branch
from
14dbcb8 to
c24eec9
Compare
2 participants
What?
As discussed in #60927 , this PR introduces a few simplifications (and deprecations) to the
Navigatorcomponent.Why?
We're simplifying the component's API ahead of promoting to stable.
How?
NavigatorToParentButton,NavigatorBackButton.goToParentanduseNavigator().goToParent()as deprecateduseNavigator().goBack()andNavigatorBackButtonwill navigate to the parent screen, instead of navigating to whatever screen was visited prior to the current one (according to the expected URL-basedpathassigned to screens)Questions
goToParentandNavigatorToParentButton?replaceoption make any sense with "go to parent"-only backwards navigation?Next steps
Testing Instructions
Navigatorshould continue to behave as ontrunkin Storybook and in the block editor