Navigator: fix isInitial, refine focusSelector logic #64786
Conversation
ciampo
added
[Type] Bug
|
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. |
ciampo
changed the title
Navigator: fix isInitial & refine focuselector logic
ciampo
changed the title
packages/components/CHANGELOG.md
Outdated
| @@ -42,6 +42,10 @@ | |||
| - `TreeSelect` | |||
| - `UnitControl` | |||
|
|
|||
| ### Bug Fixes | |||
|
|
|||
| - `Navigator`: fix isInitial & refine focus selector logic added in #64675 ([#64786](https://github.com/WordPress/gutenberg/pull/64786)). | |||
There was a problem hiding this comment.
There wasn't a release since #64675 was merged, right? Then this changelog entry describes something that a package consumer never had a chance to experience, kind of redundant.
There was a problem hiding this comment.
Good point — I've removed the commit and currently this PR doesn't add any changelog entry.
| @@ -82,25 +83,32 @@ function goTo( | |||
| return { currentLocation, focusSelectors }; | |||
There was a problem hiding this comment.
If I'm at initial location /foo and call goTo( '/foo' ), it will reset the isInitial flag and that's all. Isn't it going to fire an animation? Like the same UI sliding from right "into itself"?
By the way, all the ?.path optional chainings are not needed, .path is enough.
There was a problem hiding this comment.
If I'm at initial location /foo and call goTo( '/foo' ), it will reset the isInitial flag and that's all. Isn't it going to fire an animation? Like the same UI sliding from right "into itself"?
I don't think "to same screen" navigations have ever been supported — for the animation to work, we'd need to create a clone of the current screen to transition to.
This is true also for "parametrised" screens. For example, if we have a <Navigator.Screen path="/product/:id" />, navigating from /product/1 to product/2 won't currently trigger an animation.
I don't think this has ever been a requirement, and that's why it's not supported.
By the way, all the ?.path optional chainings are not needed, .path is enough.
Good point, I pushed a commit removing those optional chainings
ciampo
force-pushed
the
fix/navigator-provider-logic
branch
from
95da744
to
c8a3f44
Compare
* Fix isInitial logic * Use '@wordpress/warning' instead of console.warn * Improved destructuring and isInitial assignment * Refactor focus selector logic, better cleanup, lazier map copy * Remove unnecessary optional chaining — currentLocation is always defined --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
What?
Part of #59418
Follow-up to #64675
Following the changes applied in #64675, this PR brings fixes and improvements to
Navigator's internal logic following @jsnajdr 's commentsWhy?
Jarda's comments are all correct and actionable, and would improve the compoment.
How?
isInitiallogic (as highlighted in Navigator: remove location history, simplify internal logic #64675 (comment))console.warnwith@wordpress/warning(as suggested in Navigator: remove location history, simplify internal logic #64675 (comment))Testing Instructions
Both in Storybook and in the editor (Global Styles sidebar):
isInitialis only applied to the very first location that is assigned whenNavigatormounts (currently, ontrunk, it gets set totrueany time the location'spathis the same as the initial path — which is wrong)