Global styles revisions: integrate style book #56800
Merged
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
12 tasks
ramonjd
changed the title
|
Size Change: +185 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d1307e6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7242922633
|
ramonjd
added
[Type] Enhancement
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
from
e43943b
to
a6a11e4
Compare
ramonjd
commented
Dec 13, 2023
| @@ -57,6 +78,20 @@ export default function GlobalStylesSidebar() { | |||
| }, [ shouldClearCanvasContainerView ] ); | |||
|
|
|||
| const { setIsListViewOpened } = useDispatch( editSiteStore ); | |||
| const { goTo } = useNavigator(); | |||
| const loadRevisions = () => { | |||
There was a problem hiding this comment.
I've moved the revision icon in the styles panel header here to make it easier to work with style book vars.
That's the theory 😄
There was a problem hiding this comment.
Actually, this change might not be needed. Might be a okay for a follow up as it consolidates styles actions.
There was a problem hiding this comment.
I'll do this in a follow up:
ramonjd
commented
Dec 13, 2023
| const settings = useMemo( | ||
| () => ( { ...originalSettings, __unstableIsPreviewMode: true } ), | ||
| [ originalSettings ] | ||
| ); | ||
|
|
||
| const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig ); |
There was a problem hiding this comment.
This repeats what <Revisions /> does. Should perhaps abstract into a hook
There was a problem hiding this comment.
Okay, the refactor goes deep 😄
I might do it in a follow up.
The plan:
- add and export a new function
useGlobalStylesOutputWithCustomConfig( customConfig: GlobaStylesObject )or something to use-global-styles-output.js - Move and export the util
mergeBaseAndUserConfigsfrom global-styles-provider.js to use-global-styles-output.js and update all usages. - Use
useGlobalStylesOutputWithCustomConfigin Revisions and Style Book
Note to self - working diff:
Diff
diff --git a/packages/block-editor/src/components/global-styles/index.js b/packages/block-editor/src/components/global-styles/index.js
index 76a95357ba..e7f965fe24 100644
--- a/packages/block-editor/src/components/global-styles/index.js
+++ b/packages/block-editor/src/components/global-styles/index.js
@@ -9,6 +9,7 @@ export {
getLayoutStyles,
useGlobalStylesOutput,
useGlobalStylesOutputWithConfig,
+ useGlobalStylesOutputWithCustomConfig,
} from './use-global-styles-output';
export { GlobalStylesContext } from './context';
export {
diff --git a/packages/block-editor/src/components/global-styles/use-global-styles-output.js b/packages/block-editor/src/components/global-styles/use-global-styles-output.js
index 7e99eca355..37996587de 100644
--- a/packages/block-editor/src/components/global-styles/use-global-styles-output.js
+++ b/packages/block-editor/src/components/global-styles/use-global-styles-output.js
@@ -1,3 +1,8 @@
+/**
+ * External dependencies
+ */
+import deepmerge from 'deepmerge';
+
/**
* WordPress dependencies
*/
@@ -38,6 +43,7 @@ import {
setImmutably,
} from '../../utils/object';
import BlockContext from '../block-context';
+import { isPlainObject } from 'is-plain-object';
// List of block support features that can have their related styles
// generated under their own feature level selector rather than the block's.
@@ -48,6 +54,23 @@ const BLOCK_SUPPORT_FEATURE_LEVEL_SELECTORS = {
typography: 'typography',
};
+
+function isObjectEmpty( object ) {
+ return ! object || Object.keys( object ).length === 0;
+}
+
+// Copied from packages/edit-site/src/components/global-styles/global-styles-provider.js
+// @todo: this should be exported and used from here?
+export function mergeBaseAndUserConfigs( base, user ) {
+ return deepmerge( base, user, {
+ // We only pass as arrays the presets,
+ // in which case we want the new array of values
+ // to override the old array (no merging).
+ isMergeableObject: isPlainObject,
+ } );
+}
+
+
function compileStyleValue( uncompiledValue ) {
const VARIABLE_REFERENCE_PREFIX = 'var:';
const VARIABLE_PATH_SEPARATOR_TOKEN_ATTRIBUTE = '|';
@@ -1264,3 +1287,35 @@ export function useGlobalStylesOutput() {
const { merged: mergedConfig } = useContext( GlobalStylesContext );
return useGlobalStylesOutputWithConfig( mergedConfig );
}
+
+/**
+ * Like useGlobalStylesOutputWithConfig but returns global style output by
+ * merging incoming "overrides" with base config and settings.
+ *
+ * The use case is to output real-time changes to global styles based on custom configs
+ * other than `userConfig`, for example historical global styles changes (revisions).
+ *
+ * @param {Object} customConfig A custom global styles configuration.
+ * @return {Array|undefined} Array of global styles stylesheets and settings. Undefined if no styles are output.
+ */
+export function useGlobalStylesOutputWithCustomConfig( customConfig ) {
+ const { base: baseConfig } = useContext( GlobalStylesContext );
+
+ const mergedConfig = useMemo( () => {
+ if (
+ ! isObjectEmpty( customConfig ) &&
+ ! isObjectEmpty( baseConfig )
+ ) {
+ return mergeBaseAndUserConfigs( baseConfig, customConfig );
+ }
+ return {};
+ }, [ baseConfig, customConfig ] );
+
+ const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig );
+
+ if ( ! isObjectEmpty( globalStyles ) && ! isObjectEmpty( customConfig ) ) {
+ return globalStyles;
+ }
+
+ return undefined;
+}
diff --git a/packages/edit-site/src/components/revisions/index.js b/packages/edit-site/src/components/revisions/index.js
index aeb7779d06..ba1b53730d 100644
--- a/packages/edit-site/src/components/revisions/index.js
+++ b/packages/edit-site/src/components/revisions/index.js
@@ -6,52 +6,30 @@ import { __ } from '@wordpress/i18n';
import {
BlockList,
privateApis as blockEditorPrivateApis,
- store as blockEditorStore,
__unstableEditorStyles as EditorStyles,
+ store as blockEditorStore,
__unstableIframe as Iframe,
} from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';
import { useMemo } from '@wordpress/element';
-import { store as coreStore } from '@wordpress/core-data';
/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';
-import { mergeBaseAndUserConfigs } from '../global-styles/global-styles-provider';
import EditorCanvasContainer from '../editor-canvas-container';
-const { ExperimentalBlockEditorProvider, useGlobalStylesOutputWithConfig } =
- unlock( blockEditorPrivateApis );
-
-function isObjectEmpty( object ) {
- return ! object || Object.keys( object ).length === 0;
-}
+const {
+ ExperimentalBlockEditorProvider,
+ useGlobalStylesOutputWithCustomConfig,
+} = unlock( blockEditorPrivateApis );
function Revisions( { actions, userConfig, blocks } ) {
- const { baseConfig } = useSelect(
- ( select ) => ( {
- baseConfig:
- select(
- coreStore
- ).__experimentalGetCurrentThemeBaseGlobalStyles(),
- } ),
- []
- );
-
- const mergedConfig = useMemo( () => {
- if ( ! isObjectEmpty( userConfig ) && ! isObjectEmpty( baseConfig ) ) {
- return mergeBaseAndUserConfigs( baseConfig, userConfig );
- }
- return {};
- }, [ baseConfig, userConfig ] );
-
const renderedBlocksArray = useMemo(
() => ( Array.isArray( blocks ) ? blocks : [ blocks ] ),
[ blocks ]
);
-
const originalSettings = useSelect(
( select ) => select( blockEditorStore ).getSettings(),
[]
@@ -60,13 +38,8 @@ function Revisions( { actions, userConfig, blocks } ) {
() => ( { ...originalSettings, __unstableIsPreviewMode: true } ),
[ originalSettings ]
);
-
- const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig );
-
const editorStyles =
- ! isObjectEmpty( globalStyles ) && ! isObjectEmpty( userConfig )
- ? globalStyles
- : settings.styles;
+ useGlobalStylesOutputWithCustomConfig( userConfig ) || settings.styles;
return (
<EditorCanvasContainer
diff --git a/packages/edit-site/src/components/style-book/index.js b/packages/edit-site/src/components/style-book/index.js
index 2194920b18..4efd26a1f0 100644
--- a/packages/edit-site/src/components/style-book/index.js
+++ b/packages/edit-site/src/components/style-book/index.js
@@ -29,19 +29,17 @@ import { useSelect } from '@wordpress/data';
import { useResizeObserver } from '@wordpress/compose';
import { useMemo, useState, memo } from '@wordpress/element';
import { ENTER, SPACE } from '@wordpress/keycodes';
-import { store as coreStore } from '@wordpress/core-data';
/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';
import EditorCanvasContainer from '../editor-canvas-container';
-import { mergeBaseAndUserConfigs } from '../global-styles/global-styles-provider';
const {
ExperimentalBlockEditorProvider,
useGlobalStyle,
- useGlobalStylesOutputWithConfig,
+ useGlobalStylesOutputWithCustomConfig,
} = unlock( blockEditorPrivateApis );
const {
@@ -122,10 +120,6 @@ const STYLE_BOOK_IFRAME_STYLES = `
}
`;
-function isObjectEmpty( object ) {
- return ! object || Object.keys( object ).length === 0;
-}
-
function getExamples() {
// Use our own example for the Heading block so that we can show multiple
// heading levels.
@@ -206,25 +200,6 @@ function StyleBook( {
[ examples ]
);
- const { baseConfig } = useSelect(
- ( select ) => ( {
- baseConfig:
- select(
- coreStore
- ).__experimentalGetCurrentThemeBaseGlobalStyles(),
- } ),
- []
- );
-
- const mergedConfig = useMemo( () => {
- if ( ! isObjectEmpty( userConfig ) && ! isObjectEmpty( baseConfig ) ) {
- return mergeBaseAndUserConfigs( baseConfig, userConfig );
- }
- return {};
- }, [ baseConfig, userConfig ] );
-
- // Copied from packages/edit-site/src/components/revisions/index.js
- // could we create a shared hook?
const originalSettings = useSelect(
( select ) => select( blockEditorStore ).getSettings(),
[]
@@ -235,12 +210,8 @@ function StyleBook( {
[ originalSettings ]
);
- const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig );
-
settings.styles =
- ! isObjectEmpty( globalStyles ) && ! isObjectEmpty( userConfig )
- ? globalStyles
- : settings.styles;
+ useGlobalStylesOutputWithCustomConfig( userConfig ) || settings.styles;
return (
<EditorCanvasContainer
ramonjd
changed the title
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
from
7a63789
to
24e494a
Compare
There was a problem hiding this comment.
Oh, cool idea! I like how the additional style book button moves the toggling to the preview area, so that the buttons at the top right of the UI still switch between the overall area (and prevents folks from getting stuck). It means that it feels more like you're still "within" the global styles revisions while looking around 👍
One tiny UX thing I noticed: when you open the Revisions panel, focus is moved to the eye icon. Should it still be going to the close button?
2023-12-14.12.12.09.mp4
Would it be worth pinging some designers for feedback at this stage? I quite like the buttons here, but I wasn't sure if they might need a container or background to group them together, as they stand out a bit with darker site background colours. I.e. should it feel more like a toolbar, or are standalone buttons okay, UI-wise?
Other than that, it's all testing nicely for me so far! Toggling on and off Style Book and Revisions screens appears to be behaving as on trunk as far as I can tell 👍
ramonjd
added
Needs Design
|
Thank you for testing @andrewserong !
I had prepared an answer for this 😄 By virtue of I wasn't sure whether we should focus on the close button always, even if there are other "actions". Asking myself "why", I'm not convinced there is a good reason to do so. Is there a reason to skip the first tabbable element and land on a specific one? For reference here's the PR that introduced If we land on the close button, the user will have to back tab to find out about preceding actions. Is being able to close with a focussed button, on top of the I'm open for any approach, but it'd be good to articulate why. Let's say we wanted that behaviour.
Or, and I tried this, we use
Yes! Now that I have something half working, it'd be good to get some input. @jameskoster is my go-to for revisions 😄 |
I can't remember where the discussion last was, but from memory one of the arguments I've heard before is that if focus goes to a close button, then hitting enter to toggle something open means you can hit enter again to toggle it closed. If focus goes to another action, it's (potentially) harder for someone to go back. That's mitigated in this scenario by being able to press Escape to close the modal. But @jeryj might be another good person to ping for the desired behaviour of which button to focus first, I don't have a strong opinion either way 🙂 |
Yeah that sounds fair enough, thanks for raising that. Maybe, if we do focus on close, then we could enclose in a ButtonGroup with some labels/description Actually, maybe a ButtonGroup would be good any way 👍🏻 Let's see what folks say. Also happy to slide in either direction. |
|
Thanks for the PR, this seems like a reasonable feature to add. If a style revision includes only changes to an obscure block it should be easy enough to find a visual preview of that block. That said I don't love the design, but I also don't have any great alternative solutions because the long-term vision for the Styles panel still isn't 100% clear. One potential idea to try would be to make the style book more integrated in the revisions preview. IE instead of a (duplicate) eye icon, maybe display the tab bar permanently, with an additional (default) "Site" tab which displays the site: 
It would be good to get @SaxonF's thoughts on this as he's thought a lot more about the Styles panel than I have recently. |
That's so crazy, it just might work!! 😆 I like it. Thanks for the feedback! |
|
Another alternative until we figure out the styles panel. Make it so the style book can be toggled on/off in combination with revisions. ie both can be enabled. |
That was going to be my first take on it (See #55577 (comment) - about half way down the comment) - I shied away for fear of introducing too much complexity, but if the styles panel is fair game, I'll take a crack at it as well 😄 Might just work. Edit: here's what it looks like. I've pushed this commit in 9b7b858 so there's a record of it. 2023-12-15.15.58.42.mp4
The above alt-button state is a bit fiddly in term of UX so I'd like to try this idea too for comparison. |
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
from
2bbe8a0
to
9b7b858
Compare
Oh yeah, that makes much more sense. We'd just need to work out a couple of flow issues:
Plus that active button state you mentioned @ramonjd, but hopefully that's easy to fix. |
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
2 times, most recently
from
a6c93b1
to
313e344
Compare
apeatling
reviewed
Dec 18, 2023
Thanks for testing! So, the default white on black? Easy done. I'll just remove the custom styles and it should work out of the box. |
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
from
d1307e6
to
39656cc
Compare
Done, and updated description with new screencast. Agree - looks much more consistent. 🍺 |
|
Thanks for all the work on this so far, it feels very close. The only tiny nitpick I have is: If you open both Style Book and Revisions, then close Revisions, the Style Book should remain open. What do you think? |
That sounds logical to me. I'll take a shot at it today. Thanks for the feedback! |
… add a style book toggle for revisions.
…s area) will exit style book, but the revisions panel remains open. If you open style book then open revisions, style book will remain toggled on. Render the style book window immediately to avoid a flash of the underlying editor
…Style Book remains open Added e2e tests to reflect this
ramonjd
force-pushed
the
try/global-styles-revisions-show-style-book
branch
from
39656cc
to
84f05b2
Compare
jameskoster
approved these changes
Dec 20, 2023
apeatling
approved these changes
Dec 20, 2023
|
Thanks for hanging in there with me on this one, folks! |
|
Nice work! 🎉 |
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?
Hopefully resolves #55577
Part of:
Adds a button to toggle the Style Book in global styles revisions.
Why?
So users can preview global styles revisions in the Style Book.
The Style Book displays more patterns and blocks and therefore affords users the possibility of previewing specific historical styles that may not appear in the currently-loaded template, e.g., changes to the Calendar block.
How?
Adding a second "global-styles-revisions:style-book" view to the editor canvas container that is activated when the Revisions and Style Book buttons are active.
Testing Instructions
Screenshots or screencast
2023-12-19.15.55.29.mp4