-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigator: remove location history, simplify internal logic #64675
Conversation
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. |
packages/components/src/navigator/navigator-provider/component.tsx
Outdated
Show resolved
Hide resolved
73870a0
to
1f12c95
Compare
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.
Great cleanup, @ciampo!
Besides the focus management, looks and works well 👍
Seems like we'll need to do some re-architecting to let the reducer consider focusSelectors
changes that goTo()
does.
1f12c95
to
77fb59f
Compare
@tyxla focus management should be fixed. I believe this PR is ready for another (final?) round of review |
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.
Works well in my testing, both in global styles and in storybook. ✅
Code looks good too. I've left a few questions/suggestions, but I feel like none of them are really blocking and could be addressed in follow-ups if we wish.
Thanks @ciampo!
isBack && | ||
locationHistory.length > 1 && | ||
locationHistory[ locationHistory.length - 2 ].path === path; | ||
const focusSelectorsCopy = new Map( state.focusSelectors ); |
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.
Any chance we're needlessly creating a copy? Should we create a copy only if (and when) we really need to change it? Like, in the conditions below?
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 optimised the code so that we create a copy only when necessary — it's not as elegant, but it does the job
Open to improvements in a follow-up!
screens: Screen[]; | ||
locationHistory: NavigatorLocation[]; | ||
currentLocation?: NavigatorLocation; |
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.
Is this type precise now? Seems like we're never setting isInitial
to currentLocation
, and we're only setting it manually when computing the context. Should we clean up?
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.
The type was precise, but you're right about the fact that we could be setting isInitial
inside the reducer (instead of doing so in the context value). Done in 29e0fb2
Flaky tests detected in 29e0fb2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10529136439
|
console.warn( | ||
`Navigator: a screen with path ${ screen.path } already exists. | ||
The screen with id ${ screen.id } will not be added.` | ||
); |
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.
Let's use @wordpress/warning
for this, already used widely in the components package.
focusSelectorsCopy = new Map( state.focusSelectors ); | ||
} | ||
currentFocusSelector = focusSelectorsCopy.get( path ); | ||
focusSelectorsCopy.delete( path ); |
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.
Here I would enhance the logic for the case when we navigate to the path
in a isBack = false
way, and it has a focusSelector
stored. Then we don't want to focus (that's implemented currently) and also to delete the stored focusSelector
because it's no longer relevant.
Also, a little optimization opportunity: create the copy only when you really found a selector to be deleted. Currently, if .get( path )
is null
, you created the copy for nothing, it won't be mutated.
break; | ||
} | ||
|
||
if ( currentLocation?.path === state.initialPath ) { | ||
currentLocation = { ...currentLocation, isInitial: true }; | ||
} |
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.
Do you really want to do this? isInitial
should be true
only when the page is initially loaded. Then we don't want to do animations or change focus. But when later navigating to the initial page, the initial page is a page like any other, we want to run animations and to restore focus.
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.
Do you really want to do this?
No, the logic is wrong. I had already noticed it while working on #64777. I'm going to extract and refine the fix, and apply it in a separate PR
locationHistory = goTo( state, action.path, action.options ); | ||
const goToNewState = goTo( state, action.path, action.options ); | ||
currentLocation = goToNewState.currentLocation; | ||
focusSelectors = goToNewState.focusSelectors; |
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.
You can do a destructuring assignment in only you wrap it in parens so that it's not confused with a block:
( { currentLocation, focusSelection } = goTo() );
…s#64675) * Remove location history and only use current location * Deprecate `options.replace` * Make sure that two screens with the same path cannot be added * Re-implement focus restoration relying on a map with paths as keys * Re-implement the isInitial flag by relying on the initialPath * Update README * Comment map deletion out * CHANGELOG * Fix broken focus restoration by updating focusSelectors in the navigator reducer * create a copy of focusSelectors map only when necessary * Set isInitial inside the reducer, instead of on the context value --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
What?
Part of #59418
Related to #60927
Rewrite
Navigator
's internal logic, removing the location history.Why?
The location history is only an implementation detail after the changes in #63317 deprecated the "go back in history" behaviour, in favour of always navigating to the screen's parent.
Removing the code related to the location history allows the internal logic to be simpler and easier to maintain
How?
Navigator
only internally stores the current location;goBack
function inside the reducer was removed, together with some extra checks. ThegoTo
function now always creates a new location objectisInitial
flag is now computed based on theinitialPath
prop, instead of using the location history — again, there shouldn't be any changes in behaviour.replace
option is marked as deprecated, as it doesn't make sense without a location history (I don't think it was used in the editor anyway)Testing Instructions