Global styles: simplify the conditions in GlobalStylesEditorCanvasContainerLink #57144
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
ramonjd
added
[Type] Code Quality
ramonjd
commented
Dec 18, 2023
| @@ -138,7 +138,7 @@ function RevisionsButtons( { | |||
| return ( | |||
| <ol | |||
| className="edit-site-global-styles-screen-revisions__revisions-list" | |||
| aria-label={ __( 'Global styles revisions' ) } | |||
| aria-label={ __( 'Global styles revisions list' ) } | |||
There was a problem hiding this comment.
Changed because there's already a label Global styles revisions on the editor canvas container when revisions is active.
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
|
Flaky tests detected in cc8e51a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7242547453
|
andrewserong
approved these changes
Dec 18, 2023
There was a problem hiding this comment.
Nice tidying up @ramonjd!
✅ Styles in the left-hand sidebar still allows clicking into Revisions panel
✅ With the Style Book open, you can still navigate into blocks in global styles
✅ Style Book and Revisions containers close when expected
✅ Custom CSS Cmd+K command redirects to the CSS panel and closes revisions
While testing I noticed what I thought at first was a regression, but from re-testing trunk I think it's a bug that's present there. And that's that if you use the CMD+K command to go to the CSS panel, then it'll only work once until something else changes the the editorCanvasContainerView state. This PR improves the situation a bit as on trunk if you CMD+K from the Revisions panel, it won't go to the CSS panel, but with this PR it does 👍
Here's the issue where a second CMD+K to go to CSS fails. And then if you open the Style Book and try it again, it succeeds:
2023-12-18.15.11.38.mp4
That's all separate to this PR and this PR doesn't adversely affect it as far as I can tell (and improves on the situation slightly), so LGTM! ✨
Comment on lines
-258
to
255
|
|
||
| // location?.path is not a dependency because we don't want to track it. | ||
| // Doing so will cause an infinite loop. We could abstract logic to avoid | ||
| // having to disable the check later. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ editorCanvasContainerView, goTo ] ); |
There was a problem hiding this comment.
Really nice getting to remove this! 🎉
Comment on lines
+248
to
+250
| case 'global-styles-css': | ||
| goTo( '/css' ); | ||
| break; |
There was a problem hiding this comment.
TIL: so it looks like from where this one was introduced (#51637) that there's never been a separate "real" container view for the global styles CSS (i.e. a modal-like thingie that sits on top of the editor canvas), but this value for the editor canvas container view is used to enable this redirect to the css panel. That all sounds reasonable, and is working well for me in testing! (Small caveat, but I'll put it in the main review comment)
There was a problem hiding this comment.
Yes!
In fact, I spent a bit of yesterday trying to remove this as it's not really related to the editor container view.
I gathered that using setEditorCanvasContainerView() was seen a convenient way to trigger a route change in the global styles sidebar from anywhere.
The goTo() from useNavigator will only work in context, so goTo( '/css' ) doesn't nothing in the command component, so I agree it works for now.
Thanks for looking under the hood!
|
I appreciate the quick review @andrewserong!
I'll make a note to look for a fix. Good catch! |
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
…tainerLink (#57144) * Simplifying the conditions in GlobalStylesEditorCanvasContainerLink * Updating e2e tests
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?
Simplify the conditions that control the global styles sidebar routes depending on the state of the editor canvas container.
Why?
To make things more readable and testable I suppose. I came to this conclusion while working on #56800
How?
Switching to a
switchand setting a default of/, which is the root of the global styles sidebar.Adding e2e test coverage.
Testing Instructions
With some global styles revisions in your pocket (edit and save some styles in the side editor), open the Site Editor and:
i. You should be taken to the "Revisions" panel in the global styles sidebar
i. You should be taken to the "Additional CSS" panel in the global styles sidebar
There should be no regressions basically, and the following e2e tests should pass: