Skip to content
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

Try controlled navigator behavior #51915

Closed
wants to merge 3 commits into from
Closed

Conversation

youknowriad
Copy link
Contributor

Related to discussion in #51760

What?

This PR tries to support a "controlled" mode for the navigator component. There are still some open questions about the history/focus management...

I've added a storybook story to be able to test this properly, I didn't update the site editor yet to use it.

What we care about from the site editor's perspective

  • Being able to control the path from the site editor
  • We do care about focus management (going down and going to parent should ideally restore the focus properly) but we do not care about "controlling" these properties, it's fine if we find a way to keep these flags and elements and all internal.
  • We don't really care about "history", we care about parent/children navigation more.

This PR is not ready entirely, right now it supports passing the whole "location" object as a prop and receives a location in the onChange. My personal ideal API would be to only pass "path" (string) to both, the rest of the flags should ideally remain internal.

Obviously this is not ready, some unit tests are failing but curious about your thoughts @ciampo (especially around my requirements above and whether it's possible to implement that easily)

@youknowriad youknowriad added the [Feature] Navigation Component A navigational waterfall component for hierarchy of items. label Jun 26, 2023
@youknowriad youknowriad requested a review from ciampo June 26, 2023 13:39
@youknowriad youknowriad self-assigned this Jun 26, 2023
@ciampo ciampo requested a review from mirka June 26, 2023 13:49
@github-actions
Copy link

Flaky tests detected in 1584505.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5378880869
📝 Reported issues:

@ciampo ciampo requested a review from tyxla June 28, 2023 13:03
@tyxla
Copy link
Member

tyxla commented Jul 7, 2023

@youknowriad I'd like to provide some feedback on this one, but I need to get a bit more familiar with the navigator component. It's been a while since I was looking at it as part of #32923. Anyway, just wanted to let you know I'm planning to devote some time to it next week.

Copy link

github-actions bot commented Nov 2, 2023

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Feature] Navigation Component.

Read more about Type labels in Gutenberg.

1 similar comment
Copy link

github-actions bot commented Nov 2, 2023

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Feature] Navigation Component.

Read more about Type labels in Gutenberg.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased this PR to solve a few conflicts and update Storybook types.

In particular, some changes were applied in #52456 that had to be integrated in this PR too (unfortunately unit tests were not added for the new replace functionality)

This PR is not ready entirely, right now it supports passing the whole "location" object as a prop and receives a location in the onChange. My personal ideal API would be to only pass "path" (string) to both, the rest of the flags should ideally remain internal.

I'm not sure we can really do that, since the location objects include those extra pieces of information (like isBack and focusTargetSelector) which make the component work as expected even when controlled

/**
* The current path. When provided the navigator will be controlled.
*/
location?: NavigatorLocation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change path in the NavigatorLocation type to be mandatory

path: initialPath,
},
] );
>( [ location ?? { path: initialPath } ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that useControlledValue is already handling the fallback to { path: initialPath } as the default value when locationProp is not defined, I believe that this fallback is extra and can be removed

Suggested change
>( [ location ?? { path: initialPath } ] );
>( location ? [ location ] : [] );

className,
...otherProps
} = useContextSystem( props, 'NavigatorProvider' );
const [ location, updateLocation ] = useControlledValue( {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to keep both location and locationHistory internal states? I'm afraid it would cause extra re-renders and potentially some race conditions too. Maybe we can just keep the location history as a ref, and update navigatorContextValue accordingly? (I know that this would probably also require refactoring the logic around the new useEffect)

@@ -75,6 +83,90 @@ function UnconnectedNavigatorProvider(
useEffect( () => {
currentLocationHistory.current = locationHistory;
}, [ locationHistory ] );
useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a bug that causes the component to add the initial location twice to the location history when loading. This also causes the isInitial flag added in navigatorContextValue to be false even when it should be true, which causes the initial NavigatorScreen to animate on first render (when it shouldn't)

@youknowriad
Copy link
Contributor Author

I'm not sure we can really do that, since the location objects include those extra pieces of information (like isBack and focusTargetSelector) which make the component work as expected even when controlled

The problem is that I'm not sure we'll have isBack and focusTargetSelector on the wrapper component. The goal is to compute the path based on the URL and to pass that as a prop and onChange triggers a URL change directly which will result in a path being updated...

@youknowriad
Copy link
Contributor Author

The site editor moved away from the Navigator component so I don't really need this anymore.

@youknowriad youknowriad deleted the try/controlled-navigator branch April 25, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Navigation Component A navigational waterfall component for hierarchy of items.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants