Style Book: Close list view when opening the style book. #50438

juanfra · 2023-05-08T15:14:27Z

What?

Closing the list block when opening the style book.

Fixes: #50409

Why?

List view is not accessible when Style Book is opened (which is good, as it's irrelevant to Style Book).

How?

Testing Instructions

Open the site editor.
Edit styles.
Open list view when editing styles.
Launch style book.
Confirm the list view is closed when the style book is opened.

Screenshots or screencast

Screen.Recording.2023-05-08.at.17.04.56.mov

andrewserong

Thanks for working on this! Code-wise this is looking pretty good to me.

One question it raises is what to do for folks who use the Always open list view preferences setting:

For some users, they'd like the list view to be open pretty much all the time, and it could be frustrating if the list view closes and they need to re-open it again. An example of this use case is this issue: #50141 (always keep the sidebar open)

Would it be worth adding a check for the showListViewByDefault preferences setting, and only closing the list view if that value is falsy?

…ling the style book.

juanfra · 2023-05-12T08:41:41Z

Thanks so much for the review, Andrew!

Would it be worth adding a check for the showListViewByDefault preferences setting, and only closing the list view if that value is falsy?

I think that setting is meant to work (have an effect) while being in edit mode and having access to blocks. So, I'm adding a check to respect the setting when "closing" the style book and returning to "edit mode". And also respecting things when the X is clicked.

Screen.Recording.2023-05-12.at.10.26.16.mov

When the user is in the "style book" context, the list view doesn't affect the canvas, and as a result, it becomes useless. That was my interpretation of the issue, and that's why I'm closing it in that state.

andrewserong

Nicely thought through @juanfra! I think that behaviour addresses the main concern that I had, and it's testing well for me with Always open list view switched on, and switched off 👍

Just left a question about the addition of the setCanvasMode( 'edit' ); line, as it could wind up conflicting with other explorations into rendering the EditorCanvasContainer in browse mode. What do you think?

andrewserong · 2023-05-15T06:52:46Z

packages/edit-site/src/components/editor-canvas-container/index.js

@@ -66,6 +66,7 @@ function EditorCanvasContainer( { children, closeButtonLabel, onClose } ) {
 		if ( typeof onClose === 'function' ) {
 			onClose();
 		}
+		setCanvasMode( 'edit' );


Switching to editor mode when the container is closed might not always be desirable. Separately to this PR, oven in #50566, I'm exploring rendering the Style Book via the editor canvas container, but not within the edit mode. In that case, closing the Style Book shouldn't switch modes.

Is it possible to otherwise retain the behaviour in this PR without having to explicitly set the mode here?

I agree with @andrewserong - the editor canvas container component should be able to function in any editor "mode", and shouldn't therefore affect how other components use it.

What about passing your desired functionality in the onClose prop?

Thank you! sure, I will update the PR. I was trying to avoid having to duplicate code

juanfra · 2023-05-15T09:19:31Z

Thank you, @andrewserong! Yes, I added that, so it also respects the sidebar setting when the stylebook was "closed" by clicking the X. As setting the canvas mode to edit will perform the check for the sidebar setting. I thought the editor canvas container was always invoked in the edit context, and that's why I chose to set the canvas mode to edit on close.

I wasn't aware that the work for having the style book opener on the sidebar took that direction; I see some requests to change the current behavior of having the tabs and X there; maybe it's better to wait and see the final direction there to finally adjust this one.

andrewserong · 2023-05-15T23:48:47Z

I wasn't aware that the work for having the style book opener on the sidebar took that direction; I see some requests to change the current behavior of having the tabs and X there; maybe it's better to wait and see the final direction there to finally adjust this one.

Thanks, yes, it sounds like we'll most likely be hiding the tabs / close button in that PR based on feedback. If you don't mind waiting a day or two, we might have a better idea of what we need there.

On the other hand, if we merge this PR as-is, it shouldn't be too hard to make subsequent changes if we need to expand the close button to work outside of the edit mode. I don't feel too strongly about it one way or the other. I might just add @ramonjd as a reviewer here, too, in case he has opinions as he's done a bunch of work on the editor canvas container lately 🙂

andrewserong

This is looking very close to me, now! Looks like it might need a rebase due to #50682 landing.

One wrinkle this introduces, with the logic occurring in EditorCanvasContainer is that now, when the the revisions view is closed, it also closes the list view. The global styles revisions view is also a feature where the list view isn't useful (clicking on a block closes the revisions view). Would it also be worth hiding the list view when the revisions container is opened? Or, would we prefer to ignore the revisions screen altogether?

Personally, I quite like the idea of keeping consistency between the style book and global styles revisions if we can, since they use similar interfaces.

What do folks think? I might just add a couple of designers as reviewers who've been active with reviewing other style book PRs, too 🙂

ramonjd · 2023-05-17T04:02:44Z

Thanks for working on this @juanfra

One wrinkle this introduces, with the logic occurring in EditorCanvasContainer is that now, when the the revisions view is closed, it also closes the list view. The global styles revisions view is also a feature where the list view isn't useful (clicking on a block closes the revisions view). Would it also be worth hiding the list view when the revisions container is opened? Or, would we prefer to ignore the revisions screen altogether?

This is a tricky one!

I also noticed that, if I close the list view before opening the style book, the list view will reopen when I close the style book

One option that might simplify things would be to just close the list view when the container opens all the time (and not try to reinstate it). That would just required a setIsListViewOpened( false ); in <EditorCanvasContainer /> and nothing else. Might also be dodgy.

🤷

I tried to check for the state of the list view in <EditorCanvasContainer /> on dismount:

function EditorCanvasContainer( { children, closeButtonLabel, onClose } ) {
	const { editorCanvasContainerView, isListViewOpened } = useSelect(
		( select ) => ( {
			editorCanvasContainerView: unlock(
				select( editSiteStore )
			).getEditorCanvasContainerView(),
			isListViewOpened: select( editSiteStore ).isListViewOpened(),
		} ),
		[]
	);
	const { setIsListViewOpened } = useDispatch( editSiteStore );
	setIsListViewOpened( false );
	const [ shouldReopenListView, setShouldReopenListView ] =
		useState( isListViewOpened );
	useEffect( () => {
		return () => {
			if ( shouldReopenListView ) {
				setShouldReopenListView( false );
				setIsListViewOpened( true );
			}
		};
	}, [ shouldReopenListView ] );
...
}

And it works for style book and revisions, but there's a pesky render error (Cannot update a component (HeaderEditMode) while rendering a different component (EditorCanvasContainer).), which I don't have time to debug

jasmussen · 2023-05-17T07:23:12Z

Thanks for the PR. I would agree that the list view isn't that useful when you're perusing the style book.

I also understand that the "Always show the list view" toggle is causing some headaches here. But we also close the list view for mobile contexts. So regardless of the toggle, perhaps it's fine to just force it off? Mainly we should be careful adding too much complex code to handle the edgecase for the non-default toggle.

I wonder, does #50564 change any of the math here?

hanneslsm · 2023-05-17T07:38:57Z

Since @andrewserong linked my issue #50141 I'd like to chime in and agree with @jasmussen that in that case it'd be totally fine to force the list view to close. However, after closing the style book the list view should be open automatically again (it it was opened before).

jameskoster · 2023-05-17T10:49:12Z

While Style Book is currently only accessed via edit view, that won't always be the case. That is to say, it is helpful to think about it (and Styles in general) as a more independent feature. So +1 from me, the list view has no relation to the Style Book, so we should hide it accordingly.

juanfra · 2023-05-18T08:06:56Z

Thanks y'all for chiming in! I'm AFK until Tuesday next week, I'll return to this then and make the proper changes.

# Conflicts: # packages/edit-site/src/components/editor-canvas-container/index.js

juanfra · 2023-06-08T08:46:45Z

I came back to this one, and I think it's ready for a new review:

Screen.Recording.2023-06-08.at.11.43.49.mov

I adjusted things so that it closes the list view when opening the stylebook in all the scenarios I could see, and then when closing the stylebook it'll open the list view only if the setting is on.

ramonjd · 2023-06-13T02:30:09Z

packages/edit-site/src/components/sidebar-edit-mode/global-styles-sidebar.js

-							onClick={ () =>
+							onClick={ () => {
+								setIsListViewOpened(
+									isStyleBookOpened && showListViewByDefault


This PR is working well for the style book for me. Thank you!

I was wondering if, while we're at it, we could give the same treatment to the revisions panel.

I think, along with the changes in this PR, the following would do it:

diff --git a/packages/edit-site/src/components/global-styles/ui.js b/packages/edit-site/src/components/global-styles/ui.js index 218665c682..bd73785e44 100644 --- a/packages/edit-site/src/components/global-styles/ui.js +++ b/packages/edit-site/src/components/global-styles/ui.js @@ -47,6 +47,7 @@ const { Slot: GlobalStylesMenuSlot, Fill: GlobalStylesMenuFill } = function GlobalStylesActionMenu() { const { toggle } = useDispatch( preferencesStore ); + const { setIsListViewOpened } = useDispatch( editSiteStore ); const { canEditCSS, revisionsCount } = useSelect( ( select ) => { const { getEntityRecord, __experimentalGetCurrentGlobalStylesId } = select( coreStore ); @@ -71,6 +72,7 @@ function GlobalStylesActionMenu() { ); const loadCustomCSS = () => goTo( '/css' ); const loadRevisions = () => { + setIsListViewOpened( false ); goTo( '/revisions' ); setEditorCanvasContainerView( 'global-styles-revisions' ); };

If it ends up causing other problems, then don't worry about it :)

Thanks for looking into this. Sure, I will add those changes.

ramonjd · 2023-06-16T05:02:13Z

Working well for me, thanks for working on this @juanfra

For the revisions & style book views:
✅ when opening, the list view closes
✅ when closing, the list view reopens in edit mode if list view is set to open in preferences

Checked from styles sidebar (LHS in view mode) and in edit mode in the RHS settings sidebar.

I think it's good to go, but might double check with @andrewserong, who is an expert at finding edge cases 😄

andrewserong

Thanks for the ping, this is testing nicely for me, too!

✅ When opening Revisions or Style Book, the list view closes
✅ When clicking the close button or toggling the Style Book icon, closing respects the list view display setting in preferences

There are a couple of cases that aren't covered for the always open list view setting (closing the global styles sidebar via the toggle button in the top toolbar, or via the close button in the right hand sidebar), but I think the current functionality in this PR is a good place to land, as I don't think it's really worth it to add more complexity for handling that case. The current behaviour captures the main closure behaviour, and in practice, it's feeling quite nice to use to me.

LGTM, thanks for persisting with this one! ✨

juanfra · 2023-06-19T08:56:40Z

Thank you for the thoughtful reviews ✨ I'm merging this one now.

Maybe an audit for the "always open" list view setting, and how it behaves in different flows could be a nice next step

…0438) * Close list view when opening the style book. * Respect the preference for showing the list view by default when toggling the style book. * Set canvas mode to edit on close. * Use `get` instead of `isFeatureActive` which is deprecated. * Apply suggestions from code review. * Force closing list view when opening stylebook from site navigation. * Close the list view when opening the revisions.

Close list view when opening the style book.

df9fd89

priethor requested review from noisysocks and andrewserong May 11, 2023 16:27

priethor added [Type] Enhancement A suggestion for improvement. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. labels May 11, 2023

andrewserong reviewed May 12, 2023

View reviewed changes

andrewserong requested a review from richtabor May 12, 2023 00:10

juanfra added 3 commits May 12, 2023 10:17

Merge branch 'master' into fix/50409-close-list-style-book

3380684

Respect the preference for showing the list view by default when togg…

7df9385

…ling the style book.

Set canvas mode to edit on close.

be33ef9

Use get instead of isFeatureActive which is deprecated.

4f305c6

andrewserong reviewed May 15, 2023

View reviewed changes

Merge branch 'master' into fix/50409-close-list-style-book

5da875d

andrewserong requested a review from ramonjd May 15, 2023 23:48

juanfra added 2 commits May 16, 2023 18:54

Merge branch 'master' into fix/50409-close-list-style-book

0bde1a7

Apply suggestions from code review.

cf51362

andrewserong reviewed May 17, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

andrewserong requested review from jameskoster and jasmussen May 17, 2023 01:40

juanfra added 2 commits May 23, 2023 14:18

Merge branch 'master' into fix/50409-close-list-style-book

9005039

# Conflicts: # packages/edit-site/src/components/editor-canvas-container/index.js

Merge branch 'master' into fix/50409-close-list-style-book

1061ba1

Force closing list view when opening stylebook from site navigation.

b181cba

juanfra requested a review from andrewserong June 8, 2023 08:46

ramonjd reviewed Jun 13, 2023

View reviewed changes

juanfra added 2 commits June 13, 2023 11:11

Merge branch 'master' into fix/50409-close-list-style-book

b02e0e7

Close the list view when opening the revisions.

0e3f221

juanfra requested a review from ramonjd June 13, 2023 09:58

andrewserong approved these changes Jun 19, 2023

View reviewed changes

juanfra merged commit 0529628 into WordPress:trunk Jun 19, 2023

juanfra deleted the fix/50409-close-list-style-book branch June 19, 2023 08:57

github-actions bot added this to the Gutenberg 16.1 milestone Jun 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style Book: Close list view when opening the style book. #50438

Style Book: Close list view when opening the style book. #50438

juanfra commented May 8, 2023

andrewserong left a comment •

edited

Loading

juanfra commented May 12, 2023

andrewserong left a comment

andrewserong May 15, 2023

ramonjd May 16, 2023

juanfra May 16, 2023 •

edited

Loading

juanfra commented May 15, 2023

andrewserong commented May 15, 2023

andrewserong left a comment •

edited

Loading

This comment was marked as duplicate.

ramonjd commented May 17, 2023

jasmussen commented May 17, 2023

hanneslsm commented May 17, 2023 •

edited

Loading

jameskoster commented May 17, 2023

juanfra commented May 18, 2023

juanfra commented Jun 8, 2023

ramonjd Jun 13, 2023

juanfra Jun 13, 2023

ramonjd commented Jun 16, 2023

andrewserong left a comment •

edited

Loading

juanfra commented Jun 19, 2023

Style Book: Close list view when opening the style book. #50438

Style Book: Close list view when opening the style book. #50438

Conversation

juanfra commented May 8, 2023

What?

Why?

How?

Testing Instructions

Screenshots or screencast

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

juanfra commented May 12, 2023

andrewserong left a comment

Choose a reason for hiding this comment

andrewserong May 15, 2023

Choose a reason for hiding this comment

ramonjd May 16, 2023

Choose a reason for hiding this comment

juanfra May 16, 2023 • edited Loading

Choose a reason for hiding this comment

juanfra commented May 15, 2023

andrewserong commented May 15, 2023

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

ramonjd commented May 17, 2023

jasmussen commented May 17, 2023

hanneslsm commented May 17, 2023 • edited Loading

jameskoster commented May 17, 2023

juanfra commented May 18, 2023

juanfra commented Jun 8, 2023

ramonjd Jun 13, 2023

Choose a reason for hiding this comment

juanfra Jun 13, 2023

Choose a reason for hiding this comment

ramonjd commented Jun 16, 2023

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

juanfra commented Jun 19, 2023

andrewserong left a comment •

edited

Loading

juanfra May 16, 2023 •

edited

Loading

andrewserong left a comment •

edited

Loading

hanneslsm commented May 17, 2023 •

edited

Loading

andrewserong left a comment •

edited

Loading