Global styles: close stylebook if the editor canvas container slot is not filled #50086
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] Bug
|
Size Change: +142 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
…a slot fill to see if we need to render a custom header title for the editor canvas slot.
ramonjd
force-pushed
the
update/site-editor-use-fills-hook-to-check-for-private-slot
branch
from
39e4ad7
to
b151f86
Compare
ramonjd
commented
Apr 26, 2023
| [] | ||
| ); | ||
|
|
||
| const { editorMode, editorCanvasView } = useSelect( ( select ) => { |
There was a problem hiding this comment.
This change reinstates the file before #50081 was merged given that this PR uses @noisysocks's original method of checking for the existence of the slot fill to determine whether to display the header.
There was a problem hiding this comment.
Thanks for following up @ramonjd!
In testing, I noticed one slight difference between this PR and the fix in #50081 to do with whether or not the style book remains closed after closing and re-opening the global styles sidebar. On trunk, if we use either of the close buttons for the global styles sidebar, then the state for the style book being closed is preserved. However with this PR, it now remembers whether the style book was opened when re-opening the global styles sidebar:
| Trunk | This PR |
|---|---|
 |
 |
In this case, as a user, it could be expected that closing the global styles sidebar would reset its state, so that reopening it again would get you back to the normal view. What do you think?
noisysocks
approved these changes
Apr 27, 2023
|
Thanks for testing folks!
That's a good point 🤔 I just double checked on an older branch and what you've described was the state of the style book before I went ahead and started making changes 😄 so this PR reinstates that behaviour. But now you've mentioned it, I personally would kind of expect the style book to go ahead and close itself if the global styles bar was closed. What do you folks think? Similar to the way that, when I drill down into Global styles > Colors > Palettes for example, and then toggle the sidebar I'm brought back to the main screen of global styles. For the style book and the upcoming revisions UI, it does make sense to reset both when the global styles panel is closed, so what's on trunk will work for a while until a new fill is created that doesn't care. I'm wondering if there's a better way to go about it 🤔 |
|
I think it makes sense to reset, yeah. |
Same, very interesting that it worked the other way around before your fix yesterday, though! It's funny how these things often only become clearer once we're testing them in detail. I quite like the reset state behaviour, but don't feel too strongly either way.
Perhaps it'll become clearer once we have a case where we don't want the state to be cleared? 🤔 |
Very good point, which is a case for leaving it for now. I'll see if I can abstract it a bit in this PR. Which also leads me to the observation that I think we should also reset when |
…hrows in a check for canvas mode === `edit`, both of which will affect whether we reset the editor canvas container. If the global sidebar is closed or the canvas is no longer in edit mode, turn off the fills.
|
@noisysocks and @andrewserong thank you for spending time on this one. I've gone with a balance:
I'm overthinking this of course 😄 But it works. |
andrewserong
approved these changes
Apr 27, 2023
There was a problem hiding this comment.
Thanks for following up, this looks like a good balance to me!
Added an extra check for 'edit' === canvasMode in order to close the stylebook when NOT in edit mode
I like that inclusion, too, as it means clicking the W icon escapes the user out of the style book, too. Nice that we're prevent folks from getting stuck!
LGTM ✨
ramonjd
deleted the
update/site-editor-use-fills-hook-to-check-for-private-slot
branch
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.
!! t.
What?
An enhancement to to #50081 which reinstates the original method of checking for a slot fill to see if we need to render a custom header title for the editor canvas slot.
It also checks the canvasMode so we can reset the stylebook slot.
Why and how?
Previously we could check that the stylebook was open by using
useSlotFillsto see if the stylebook slot was filled.Since changing over to a private slot fill in #49973, we now have to get the key using
privateKey.Testing Instructions
Screenshots or screencast