Styles Navigation Screen: close style book using location #51365
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
…ew` instead selector to avoid e2e fails. playwright runs faster than the useEffect resetting the value.
andrewserong
added
the
[Type] Code Quality
kevin940726
approved these changes
Jun 9, 2023
There was a problem hiding this comment.
LGTM as long as it stops the flakiness 👍
We should probably look into an alternative than using a useEffect though, it seems like an anti-pattern to me. My intuition would be using an event callback from the navigator to handle this instead. c.c. @Mamaduka if you're interested!
andrewserong
approved these changes
Jun 9, 2023
There was a problem hiding this comment.
Thanks for putting up this fix @ramonjd!
✅ The behaviour where clicking out of the Styles screen with the style book opened, and then clicking back into it resets the container state as on trunk
✅ e2e tests are now passing locally for me:
LGTM! ✨
We should probably look into an alternative than using a useEffect though, it seems like an anti-pattern to me. My intuition would be using an event callback from the navigator to handle this instead.
Good point, it does sound like it'd be better if we could fire a change when the navigation happens rather than the next cycle in a useEffect. For what it's worth, it looks like useSyncPathWithURL is also useEffect-based, so not too sure what that means for changing how we hook into navigation changes?
Either way, this looks good to merge to me, for now!
|
Size Change: +6 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
andrewserong
deleted the
update/site-editor-sidebar-canvas-container-reset
branch
|
Flaky tests detected in 584d9ea. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5218966276
|
|
Thanks for fixing the flaky tests, @ramonjd! I agree with @kevin940726 and @andrewserong regarding |
|
What do folks think is required here? I'm not sure I'll have to time to look into it, but just asking for the record.
A naive approach could be passing a callback when calling
Or maybe const { onLocationChange } = useNavigator();
onLocationChange( ( newLocation, previousLocation ) => {
doSomething();
} );
// in navigator-provider
const onLocationChange = useCallback(
( callback ) => {
callback( newLocation, previousLocation );
},
[ newLocation, previousLocation ]
);Or something along those lines.... ? |
|
I think I mentioned something related at the beginning when introducing the Navigator component but unfortunately it didn't go anywhere 😅 . Basically what I imagine is a useEffect(() => {
return history.listen(({ location }) => {
doSomething();
});
}, []);It seems simpler to me than trying to figure out when a custom hook would run. Just a simple but powerful API for advanced usage. If we were using the |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…51365) * Using location to determine whether to reset `editorCanvasContainerView` instead selector to avoid e2e fails. playwright runs faster than the useEffect resetting the value. * path is a property. * adding location?.path to dep array
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.
(Maybe) Fixes #50799, fixes #50807, fixes #50837, fixes #50838, fixes #50839
What?
Using location to determine whether to reset
editorCanvasContainerViewinstead selector to avoid e2e fails.Why?
playwright runs faster than the useEffect resetting the value. (thanks @kevin940726 !)
Testing Instructions
npm run test:e2e:playwright -- --headed site-editor/style-book.spec