Global styles: improve navigation logic for revisions screen #65946
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: -62 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
|
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. |
tyxla
reviewed
Oct 8, 2024
Comment on lines
271
to
274
| if ( isRevisionsOpen ) { | ||
| goTo( '/' ); | ||
| goTo( '/', { isBack: true } ); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Should we just let the case fall through to the default one?
There was a problem hiding this comment.
You could also keep the comment if you find it useful:
case 'style-book':
/*
* The stand-alone style book is open
* and the revisions panel is open,
* close the revisions panel.
* Otherwise keep the style book open while
* browsing global styles panel.
*
* Falling through as it matches the default scenario.
*/
default:
// In general, if the revision screen is in view but the
// `editorCanvasContainerView` is not a revision view, close it.
// This also includes the scenario when the stand-alone style
// book is open, in which case we want the user to close the
// revisions screen and browse global styles.
if ( isRevisionsOpen ) {
goTo( '/', { isBack: true } );
}I'm fine with omitting it or keeping it, whatever you prefer.
Comment on lines
+72
to
+73
| // The actual code that triggers the revisions screen to navigate back | ||
| // to the home screen in in `packages/edit-site/src/components/global-styles/ui.js`. |
ciampo
force-pushed
the
fix/global-styles-revisions-navigator
branch
from
43f1d32 to
8dc48ba
Compare
ciampo
commented
Oct 8, 2024
There was a problem hiding this comment.
All of the goTo calls in this file are no-ops, because useNavigator is being called in a component that is not a child of a Navigator component.
|
As i was addressing some feedback, I noticed a few places where calling
I confess that I'm a bit loss with respect to what is the expected behaviour of all possible interactions between global styles, revisions, style book, and additional CSS styles. Maybe @ramonjd @andrewserong @jameskoster can help understand this expected behaviour, so that we can clean up the logic? |
|
The Styles panel was originally built with a narrow scope and over time has become a bit of a Frankenstein's monster. The whole iA needs a rethink (#47369), and until that's done I'm hesitant to give any suggestions that may make the monster larger. There's some related work in #65619, demonstrating this is a big project that needs careful thought. Sorry for not being much help 🙈 |
|
Got it. I'll make changes to the best of my knowledge for noe, and we can hopefully merge this PR as it cleans up navigator-related code quite a bit |
tyxla
reviewed
Oct 9, 2024
| @@ -86,21 +86,17 @@ export default function GlobalStylesSidebar() { | |||
| }, [ shouldClearCanvasContainerView ] ); | |||
|
|
|||
| const { setIsListViewOpened } = useDispatch( editorStore ); | |||
| const { goTo } = useNavigator(); | |||
There was a problem hiding this comment.
Great catch 👏
FWIW we should also remove the useNavigator import.
There was a problem hiding this comment.
I just did this while testing. 👍🏻
ramonjd
approved these changes
Oct 9, 2024
ciampo
force-pushed
the
fix/global-styles-revisions-navigator
branch
from
9282f74 to
d53f641
Compare
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ss#65946) * Global styles: improve navigation logic for revisions screen * Merge style book case into default, add comment * Remove unnecessary goTo calls * Remove useNavigator import * Apply feedback --------- Co-authored-by: ciampo <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: jameskoster <[email protected]>
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.
What?
Part of #65794
Tweak the code in the global styles sidebar, improving the logic around navigating to and away from the revisions screen.
Why?
Navigating to/from the revisions screen wasn't always triggering the correct animation, and sometimes the exit animation was playing twice.
Note
Navigating to the revisions screen can still trigger a double animation, but I believe this is caused by how the screen itself handles data fetching updates via a mix of data store selectors and internal state, which causes the component to re-render and potentially re-trigger the CSS entry animation. This PR doesn't aim at improving that aspect — to improve it, we should probably refactor the revisions screen's internal code more substantially, similarly to the work that @jsnajdr did in #65564
How?
Since the button that shows/hides the screen is actually outside of
Navigator, it can't useNavigator.ButtonoruseNavigatordirectly. Instead, interacting with the button changes a value in the data store, and then code in the global styles sidebar react to those changes to programmatically navigate to a new screen.This PR removes any similar logic in the revisions screen itself and moves any needed logic to the file hosting the root
Navigatorcomponent, next to where other similar logic is.This PR also add the
isBackflag when navigating to the home screen, which makes the animation more aligned with the user's expectations.Testing Instructions
trunk, it navigates forward)trunk, and fixing it is out of the scope of this PR.Screenshots or screencast
Kapture.2024-10-08.at.14.54.33.mp4
Kapture.2024-10-08.at.14.58.40.mp4