Add command for Style Book and improve Style Revisions command #58143
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: +264 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Jan 24, 2024
There was a problem hiding this comment.
Thanks for the PR! I think these suggestions make sense to me too.
"Close Style Book", and "Close Style Revisions"
Both of these close commands seem to simply close the Global Style itself. However, I think Style Book and revisions are not exclusive, so I feel that the Global Styles should remain open.
Perhaps the implementation of this part will be helpful. What do you think?
The screencast below shows the Revision and Style Book buttons working independently.
2dc2abd8e9d1e862dc00bec69974e32f.mp4
Another thing to solve is to disable these commands from being executed in the classic theme. Currently, the classic theme that supports template parts gives us access to some parts of the Site Editor, but classic themes do not support Style Book.
This issue can be reproduced by following these steps:
- Access
http://localhost:8889/wp-admin/instead ofhttp://localhost:8888/wp-admin/ - Activate the Emptyhybryd theme
- Access Appearance > Template parts
- Run "Open Style Book" from the command palette
You should be able to reuse the implementation here to deal with this problem.
| @@ -288,6 +310,91 @@ function useGlobalStylesOpenRevisionsCommands() { | |||
| isEditorPage, | |||
| getCanvasMode, | |||
| setCanvasMode, | |||
| closeGeneralSidebar, | |||
There was a problem hiding this comment.
You will also need to adjust the dependencies for the useMemo hook. Your code editor should tell you what dependencies you need and don't need.
Comment on lines
+247
to
+249
| const { getCanvasMode, getEditorCanvasContainerView } = unlock( | ||
| useSelect( editSiteStore ) | ||
| ); |
There was a problem hiding this comment.
My understanding is that instead of unlocking useSelect itself, you will need to do it inside the useSelect hook. You will probably need to modify the existing useSelect hook to look like this:
const { canvasMode, hasRevisions, isStyleRevisionsOpened } = useSelect(
( select ) => {
const { getEntityRecord, __experimentalGetCurrentGlobalStylesId } =
select( coreStore );
const { getCanvasMode, getEditorCanvasContainerView } = unlock(
select( editSiteStore )
);
const globalStylesId = __experimentalGetCurrentGlobalStylesId();
const globalStyles = globalStylesId
? getEntityRecord( 'root', 'globalStyles', globalStylesId )
: undefined;
return {
canvasMode: getCanvasMode(),
hasRevisions:
!! globalStyles?._links?.[ 'version-history' ]?.[ 0 ]
?.count,
isStyleRevisionsOpened:
'global-styles-revisions' ===
getEditorCanvasContainerView(),
};
},
[]
);
There was a problem hiding this comment.
I agree with the getEditorCanvasContainerView move inside the useSelect, as it's being used there.
getCanvasMode though is used in a callback function, so it's fine to have outside and call it when needed to get the latest value.
There was a problem hiding this comment.
If you have a few moments, feel free to take over the PR to get this in.
Comment on lines
+329
to
+331
| const { getCanvasMode, getEditorCanvasContainerView } = unlock( | ||
| useSelect( editSiteStore ) | ||
| ); |
There was a problem hiding this comment.
Similar to this comment
|
@ramonjd I added you as a reviewer. Because I assume you have deep experience with Style Book and revision specifications 🙏 |
ramonjd
reviewed
Jan 29, 2024
There was a problem hiding this comment.
A style book command is a great addition in my opinion. Thank you!
I'm a bit skeptical of the need to allow for open/close toggling. I'm running on the idea that folks will activate or open things as they need, using commands in a forward manner.
The style book and revisions components immediately place focus on the close button anyway.
Do we have open/close toggling elsewhere?
| useSelect( () => { | ||
| return { | ||
| isStyleBookOpened: 'style-book' === canvasContainerView, | ||
| isRevisionsStyleBookOpened: |
There was a problem hiding this comment.
I'm thinking it might make the open/close logic more straight forward for users to have the choice to toggle revisions regardless of whether the style book view is open.
So something like:
isStyleBookOpened: 'style-book' === canvasContainerView,
isRevisionsOpened: canvasContainerView.startsWith( 'global-styles-revisions' ),
So "Open/Close Revisions" ignores which type of preview we're showing - the commands will either open or close revisions.
"Open/Close style book" would target the style book specifically.
I mention it because the command "Close style book" is present in the context of revisions:
I suppose it's okay to have a command that closes the style book revisions "view" too - I just have a niggling feeling the third state is unnecessary for the user.
| } | ||
| setEditorCanvasContainerView( | ||
| isStyleBookOpened ? undefined : 'style-book' | ||
| ); |
There was a problem hiding this comment.
To allow opening from the site editor "view" sidebar, I think we'd also need to account for the canvas view mode and set it to "edit" if it isn't.
Similar to how the styles revisions callback works:
if ( isEditorPage && getCanvasMode() !== 'edit' ) {
setCanvasMode( 'edit' );
}
If we allow you to command it to open, we should have one to close it as well. It's essentially the same command, but the inverse.
Yes, implemented for most of the UI-changing commands. Some are "Toggle..." while others are more specific, like "Enter Distraction Free" and "Exit Distraction Free". |
|
I also think that bidirectional actions should be supported, but I think the question is whether to use "Toggle" or "Open/Close" or some other terminology. Below is a list of commands that support bidirectional actions in the Site Editor.
In my opinion, "Open/Close" is suitable for both Style Book and Revisions. Also, I think this problem is probably related to usability, so I will send a ping to @afercia. |
|
Thanks for the ping. The verb 'Toggle' shoud be avoided. See: #42492 It's hardly translatable in other languages. Typically, translators resort to using a verb pair e.g.: I'd tend to think the command label should be contextual. While I personally don't like contextual labels for buttons or other UI controls, I'd think the command palette should only show commands that make sense in the current, specific, state of the UI. While the underlying action is 'toggle' a panel/sidebar/whatever, ideally the label of the command should be contextual. For example:
Re: |
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?
Adds a command for Style Book, while also editing the Style Revisions command so they both work nicely together.
Run the "Open Style Book", "Open Style Revisions", "Close Style Book", and "Close Style Revisions" commands.
Screenshots or screencast