Navigator: add basic history support, enable advanced focus restoration
#37223
Conversation
This file contains 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
|
Size Change: +1.25 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
try/navigator-restore-focus-on-back-link
branch
from
f6b6335
to
869bcde
Compare
ciampo
added
[Feature] Component System
ciampo
force-pushed
the
try/navigator-restore-focus-on-back-link
branch
2 times, most recently
from
6c65d06
to
4493d59
Compare
ciampo
changed the title
Navigator: basic history support, enable focus restorationNavigator: add basic history support, enable focus restoration
ciampo
changed the title
Navigator: add basic history support, enable focus restorationNavigator: add basic history support, enable advanced focus restoration
ciampo
requested review from
mtias,
mirka,
jasmussen,
youknowriad and
diegohaz
youknowriad
reviewed
Dec 13, 2021
youknowriad
reviewed
Dec 13, 2021
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Dec 13, 2021
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Dec 13, 2021
youknowriad
reviewed
Dec 13, 2021
packages/edit-site/src/components/global-styles/navigation-button.js
Outdated
Show resolved
Hide resolved
|
Not sure if you're aware of this, but we've recently done other approach to try to implement a custom client side router in the side editor. See #36488 for the basic implementation and #37314 for focus management and some refactor. The approach we take there is to make it browser-first, and use the Even though we currently don't have the need to implement a real browser-based client side router, I think it's still good to take it into account in case we ever have the requirement to do so. Maybe we could combine the two into this component and learn from each other? |
|
@kevin940726 yeah browser navigation has actually been part of the initial implementation of the Navigator component but we felt that it's not something that is needed in all use-cases and not something that should be backed into a UI component, for this reason we decided to built a generic UI component and still have an API that allow syncing with browser history if needed for non-mobile/web apps like edit-site. The original prototype here #32923 has a mechanism to sync navigation state with history that could serve as inspiration. |
|
Hey @kevin940726 and @youknowriad , I created #37355 to discuss the interaction between |
15 tasks
…`isFirstRender` in the `NavigatorScreen`
…hing a new location
ciampo
force-pushed
the
try/navigator-restore-focus-on-back-link
branch
from
11a72bb
to
ba08a42
Compare
youknowriad
reviewed
Dec 15, 2021
packages/components/src/navigator/navigator-back-link/component.tsx
Outdated
Show resolved
Hide resolved
|
I don't have any more feedback in this PR, I'm really satisfied with how this is shaping up. Would be good to get a bit more testing and 🚢 |
This is great to hear! I was initially thinking of splitting the changes in this PR into 3 separate PRs for ease of review/track-ability — they could be:
Would you be ok with that? Otherwise I can just continue the work in this PR (adding unit tests, refining docs...) before asking other folks for a final review. |
|
@ciampo whatever works best for you. The current size of the PR is still manageable/easily reviewable but if you want to ship some things more quickly, go split it :) |
|
In that case, I'm going to close this PR. I will open a few smaller PRs instead — thank you again for your feedback so far! |
7 tasks
7 tasks
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Following the conversation in #37063, this PR is an exploration into enabling focus restoration when navigating back
Changes:
Navigatorcomponent, especially the shape of the context object: it now exposeslocation(the current location),push(used to navigate to a new location), andpop(used to navigate to the previous location)NavigatorScreenwould focus its first tabbable element when mountedquerySelector, and it is the responsibility of the developer to make sure that each link can be correctlyselectedby providing a working selector.TODO:
NavigatorLinkNavigator's unit testsNavigatorREADME and JSDocs (including the@examples)How has this been tested?
Screenshots
Storybook
navigator-focus-restoration.mp4
Preferences Modal
preference-modal-focus-restoration.mp4
Global Styles Sidebar
global-styles-sidebar-focus-restoration.mp4
Types of changes
New feature (technically not breaking, since the component is experimental)
Checklist:
*.native.jsfiles for terms that need renaming or removal).