Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give style book its own route so it can be linked to directly. #67811

Merged
merged 5 commits into from
Dec 18, 2024
+81 −65
Merged

Give style book its own route so it can be linked to directly. #67811

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca4b740
Give style book its own route so it can be linked to directly.
tellthemachines Dec 11, 2024
fc32d39
Fix paths to and from global styles.
tellthemachines Dec 12, 2024
72ba66d
Use query instead of path
tellthemachines Dec 16, 2024
67c9d71
Fix path
tellthemachines Dec 17, 2024
005a47e
Effect for editor settings update
tellthemachines Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 26 additions & 46 deletions packages/edit-site/src/components/sidebar-global-styles-wrapper/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,35 +15,43 @@ import { seen } from '@wordpress/icons';
import GlobalStylesUI from '../global-styles/ui';
import Page from '../page';
import { unlock } from '../../lock-unlock';
import StyleBook from '../style-book';
import { STYLE_BOOK_COLOR_GROUPS } from '../style-book/constants';

const { useLocation, useHistory } = unlock( routerPrivateApis );

const GlobalStylesPageActions = ( {
isStyleBookOpened,
setIsStyleBookOpened,
path,
} ) => {
const stylebookPath = addQueryArgs( path, { preview: 'stylebook' } );
const history = useHistory();
return (
<Button
isPressed={ isStyleBookOpened }
icon={ seen }
label={ __( 'Style Book' ) }
onClick={ () => {
setIsStyleBookOpened( ! isStyleBookOpened );
const updatedPath = ! isStyleBookOpened
? path.replace( '/styles', stylebookPath )
: path.replace( stylebookPath, '/styles' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these replacements aren't quite right, as they don't take into account any subroutes or params of /styles, e.g., /styles?section=%2Fvariations

To test, navigation to styles > browse styles, then activate the style book. Try to toggle it off or navigation back to the main menu.

Kapture.2024-12-17.at.12.18.00.mp4

Would adding and removing the preview query arg be enough?

				const updatedPath = ! isStyleBookOpened
					? stylebookPath
					: removeQueryArgs( path, 'preview' );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh maybe, let me try!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it worked 😄 thanks!

// Navigate to the updated path.
history.navigate( updatedPath );
} }
size="compact"
/>
);
};

export default function GlobalStylesUIWrapper() {
/**
* Hook to deal with navigation and location state.
*
* @return {Array} The current section and a function to update it.
*/
export const useSection = () => {
const { path, query } = useLocation();
const history = useHistory();
const { canvas = 'view' } = query;
const [ isStyleBookOpened, setIsStyleBookOpened ] = useState( false );
const isMobileViewport = useViewportMatch( 'medium', '<' );
const [ section, onChangeSection ] = useMemo( () => {
return useMemo( () => {
return [
query.section ?? '/',
( updatedSection ) => {
Expand All @@ -55,6 +63,16 @@ export default function GlobalStylesUIWrapper() {
},
];
}, [ path, query.section, history ] );
};

export default function GlobalStylesUIWrapper() {
const { path } = useLocation();

const [ isStyleBookOpened, setIsStyleBookOpened ] = useState(
path.includes( 'preview=stylebook' )
);
const isMobileViewport = useViewportMatch( 'medium', '<' );
const [ section, onChangeSection ] = useSection();

return (
<>
Expand All @@ -64,6 +82,7 @@ export default function GlobalStylesUIWrapper() {
<GlobalStylesPageActions
isStyleBookOpened={ isStyleBookOpened }
setIsStyleBookOpened={ setIsStyleBookOpened }
path={ path }
/>
) : null
}
Expand All @@ -75,45 +94,6 @@ export default function GlobalStylesUIWrapper() {
onPathChange={ onChangeSection }
/>
</Page>
{ canvas === 'view' && isStyleBookOpened && (
<StyleBook
enableResizing={ false }
showCloseButton={ false }
showTabs={ false }
isSelected={ ( blockName ) =>
// Match '/blocks/core%2Fbutton' and
// '/blocks/core%2Fbutton/typography', but not
// '/blocks/core%2Fbuttons'.
section ===
`/blocks/${ encodeURIComponent( blockName ) }` ||
section.startsWith(
`/blocks/${ encodeURIComponent( blockName ) }/`
)
}
path={ section }
onSelect={ ( blockName ) => {
if (
STYLE_BOOK_COLOR_GROUPS.find(
( group ) => group.slug === blockName
)
) {
// Go to color palettes Global Styles.
onChangeSection( '/colors/palette' );
return;
}
if ( blockName === 'typography' ) {
// Go to typography Global Styles.
onChangeSection( '/typography' );
return;
}

// Now go to the selected block.
onChangeSection(
`/blocks/${ encodeURIComponent( blockName ) }`
);
} }
/>
) }
</>
);
}
4 changes: 2 additions & 2 deletions packages/edit-site/src/components/site-editor-routes/stylebook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const stylebookRoute = {
) }
/>
),
preview: <StyleBookPreview />,
mobile: <StyleBookPreview />,
preview: <StyleBookPreview isStatic />,
mobile: <StyleBookPreview isStatic />,
},
};
6 changes: 5 additions & 1 deletion packages/edit-site/src/components/site-editor-routes/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import Editor from '../editor';
import { unlock } from '../../lock-unlock';
import SidebarNavigationScreenGlobalStyles from '../sidebar-navigation-screen-global-styles';
import GlobalStylesUIWrapper from '../sidebar-global-styles-wrapper';
import { StyleBookPreview } from '../style-book';

const { useLocation } = unlock( routerPrivateApis );

Expand All @@ -30,7 +31,10 @@ export const stylesRoute = {
areas: {
content: <GlobalStylesUIWrapper />,
sidebar: <SidebarNavigationScreenGlobalStyles backPath="/" />,
preview: <Editor />,
preview( { query } ) {
const isStylebook = query.preview === 'stylebook';
return isStylebook ? <StyleBookPreview /> : <Editor />;
},
mobile: <MobileGlobalStylesUI />,
},
widths: {
Expand Down
57 changes: 43 additions & 14 deletions packages/edit-site/src/components/style-book/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import {
} from './categories';
import { getExamples } from './examples';
import { store as siteEditorStore } from '../../store';
import { useSection } from '../sidebar-global-styles-wrapper';
import { STYLE_BOOK_COLOR_GROUPS } from '../style-book/constants';

const {
ExperimentalBlockEditorProvider,
Expand Down Expand Up @@ -346,33 +348,60 @@ function StyleBook( {
/**
* Style Book Preview component renders the stylebook without the Editor dependency.
*
* @param {Object} props Component props.
* @param {string} props.path Path to the selected block.
* @param {Object} props.userConfig User configuration.
* @param {Function} props.isSelected Function to check if a block is selected.
* @param {Function} props.onSelect Function to select a block.
* @param {Object} props Component props.
* @param {Object} props.userConfig User configuration.
* @param {boolean} props.isStatic Whether the stylebook is static or clickable.
* @return {Object} Style Book Preview component.
*/
export const StyleBookPreview = ( {
path = '',
userConfig = {},
isSelected,
onSelect,
} ) => {
export const StyleBookPreview = ( { userConfig = {}, isStatic = false } ) => {
const siteEditorSettings = useSelect(
( select ) => select( siteEditorStore ).getSettings(),
[]
);
// Update block editor settings because useMultipleOriginColorsAndGradients fetch colours from there.
dispatch( blockEditorStore ).updateSettings( siteEditorSettings );

const [ section, onChangeSection ] = useSection();
Copy link
Member

@ramonjd ramonjd Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about this hook placement triggers the following warning:

Cannot update a component (`BlockPreview`) while rendering a different component (`StyleBookPreview`). To locate the bad setState() call inside `StyleBookPreview`

For example, when navigating to an individual block's global style controls, and activating the style book.

It means we might have to wrap the dispatch in a useEffect so the settings are updated after render:

	useEffect( () => {
		dispatch( blockEditorStore ).updateSettings( siteEditorSettings );
	}, [ siteEditorSettings ] );

But I'm unsure of the side-effects - will it preserve the original intention to get the updated colors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that was merged in the classic stylebook PR though I'm not seeing the same error on trunk. I'll investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the error is caused by this PR rendering the StyleBookPreview component simultaneously with the global styles sidebar, and it seems to be complaining about the fact that we're updating the block editor settings inside StyleBookPreview while at the same time using those settings to render the BlockPreview inside the global styles sidebar. That's why the error only occurs when a specific block is selected in global styles and we click over to the stylebook view.

Wrapping the settings update in a useEffect does seem to fix the issue, so I'll add that in for now.

What I don't understand though is why we need to do this settings update in here at all. With the stylebook that renders inside the editor, the block editor settings are already up to date, but I can't work out whereabouts they're being updated.

Another thing to consider is: should we be depending on the block editor settings here, or the site editor ones? Perhaps instead of using useMultipleOriginColorsAndGradients we should have an equivalent function that grabs the color palettes from the site editor settings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't understand though is why we need to do this settings update in here at all.

I'm not sure either 🤔 I assumed it's because useMultiOriginPalettes needed the updated values here, but I commented the update line out and can't discern any difference when I update the palette in global styles and apply it to a block (the style book updates still).

Wasn't the update added as part of the class style book? #66851

I am missing something, that much is clear.

Another thing to consider is: should we be depending on the block editor settings here, or the site editor ones?

I'd say site settings is right, because the site editor is initialized (and therefore the site editor store) with preloaded theme styles settings.

Perhaps instead of using useMultipleOriginColorsAndGradients we should have an equivalent function that grabs the color palettes from the site editor settings?

Conceptually, that sounds legit to me, but I haven't gone deeper than inspecting useMultipleOriginColorsAndGradients and useSettings . The context is always going to be global styles/theme json, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context is always going to be global styles/theme json, right?

It goes a bit beyond theme.json because for classic themes it also picks up anything defined in the editor-color-palette and editor-gradient-presets theme supports. But that's when those palettes are in the block editor settings already.

I think it might be that the block editor settings only get updated when an actual block editor renders. In which case, if we're in the site preview, it makes sense that block editor settings aren't up to date, but site editor settings (which are essentially the same shape) already are.


const isSelected = ( blockName ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all beside the point, but I was looking into abstracting the stylebook component further so it can be dropped into the edit and view contexts without knowing too much about its own props.

https://github.com/WordPress/gutenberg/pull/67660/files#diff-ecd3155ef49b2da19da151cb9ac6813db5a887e4e7bd4ab1ca44b41ddba97b8b

I mean, this works too 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if long term we'll still need the style book in the edit context at all.

Reading through #66719, it seems the main reason global styles should stay in that right hand side sidebar in the editor is so folks can tweak styles while building their templates. While revisions should probably be accessible from wherever global styles can be edited, the style book is a more high-level tool so I'd question the usefulness of keeping it in that right hand sidebar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if long term we'll still need the style book in the edit context at all.

👍🏻 I'd be for this if only to simplify the all the edit canvas view juggling etc we do.

// Match '/blocks/core%2Fbutton' and
// '/blocks/core%2Fbutton/typography', but not
// '/blocks/core%2Fbuttons'.
return (
section === `/blocks/${ encodeURIComponent( blockName ) }` ||
section.startsWith(
`/blocks/${ encodeURIComponent( blockName ) }/`
)
);
};

const onSelect = ( blockName ) => {
if (
STYLE_BOOK_COLOR_GROUPS.find(
( group ) => group.slug === blockName
)
) {
// Go to color palettes Global Styles.
onChangeSection( '/colors/palette' );
return;
}
if ( blockName === 'typography' ) {
// Go to typography Global Styles.
onChangeSection( '/typography' );
return;
}

// Now go to the selected block.
onChangeSection( `/blocks/${ encodeURIComponent( blockName ) }` );
};

const [ resizeObserver, sizes ] = useResizeObserver();
const colors = useMultiOriginPalettes();
const examples = getExamples( colors );
const examplesForSinglePageUse = getExamplesForSinglePageUse( examples );

const { base: baseConfig } = useContext( GlobalStylesContext );
const goTo = getStyleBookNavigationFromPath( path );
const goTo = getStyleBookNavigationFromPath( section );

const mergedConfig = useMemo( () => {
if ( ! isObjectEmpty( userConfig ) && ! isObjectEmpty( baseConfig ) ) {
Expand Down Expand Up @@ -404,8 +433,8 @@ export const StyleBookPreview = ( {
settings={ settings }
goTo={ goTo }
sizes={ sizes }
isSelected={ isSelected }
onSelect={ onSelect }
isSelected={ ! isStatic ? isSelected : null }
onSelect={ ! isStatic ? onSelect : null }
/>
</BlockEditorProvider>
</div>
Expand Down
Loading