Give style book its own route so it can be linked to directly. #67811
Conversation
tellthemachines
added
[Type] Enhancement
|
Size Change: +63 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
tellthemachines
force-pushed
the
try/stylebook-path
branch
from
1572726 to
e308ab0
Compare
tellthemachines
requested review from
ramonjd,
youknowriad and
jorgefilipecosta
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| 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(); | ||
|
|
||
| const isSelected = ( blockName ) => { |
There was a problem hiding this comment.
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.
I mean, this works too 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| export const stylesStylebookRoute = { | ||
| name: 'styles-stylebook', | ||
| path: '/styles-stylebook', |
There was a problem hiding this comment.
Do we need a second stylebook route?
What I'm wondering is if /styles could work with sub-routes to make it more flexible later if further subroutes are added (I'm thinking revisions).
The route would end up looking like:
?p=/styles/style-book
areas: {
content( { query, params } ) {
console.log( { query, params } );
// params.subRoute, e.g., `style-book`
// query.section, e.g., `/typography`
// Now handle the logic in the component as per usual
return (
<GlobalStylesUIWrapper
subRoute={ params.subRoute }
section={ query.section }
/>
);
},There was a problem hiding this comment.
Revisions is also meant to work with the style book though? How would that look if they're both sub-routes of styles? Could revisions instead be a sub-route of either styles or styles-stylebook?
I don't feel strongly about either option. Mostly at this point I'm thinking about the further splitting up of stylebook into a few sub-routes equivalent to the tabs in the older version. But that should still work if stylebook is itself a sub-route so 🤷
There was a problem hiding this comment.
I think we need to think about this more holistically. The way I think about routes is that I try to map these to navigation menus:
- The "root" sidebar of the site editor should represent the top level routes
/some-top-level-section - If there's a sub-menu within one of these section, I should create a sub route
/top-level/second-level - If there's multiple "modifiers" that are not necessarily a navigation menu (like switching the "mode" or "layout" or things like that) and we want this state to be url accessible, I should be using "query")
/top-level/second-level?modifier=something
Based on this, you should be able to make the right call. I'm not wrong, today the "style book" is part of a dropdown where we switch what to "visualize" when editing styles. So it seems close to me to a "modifier" than a "submenu", so I'd probably use query: /styles?preview=style-book or something.
There was a problem hiding this comment.
Ok trying to think this through.
What we have right now is a top level route at the root of the site editor: /site-editor.php and then we're using queries for everything else, e.g. Pages: site-editor.php?p=%2Fpage, Styles: site-editor.php?p=%2Fstyles.
We kind of have submenus already in that within Pages we have Published, Drafts etc. I guess within Styles we could consider the main global styles sections: Colors, Typography etc. to also be submenus. But we're not using /top-level/second-level URL structure for those, we're just using query parameters.
Do we plan to change the overall URL structure for these pages anytime soon? Otherwise, keeping to the logic of the current structure, both submenus and modifiers should be expressed as query parameters.
Does that make sense, or am I missing anything here? I'm not really invested in any solution, just trying to understand what will work best overall!
There was a problem hiding this comment.
What we have right now is a top level route at the root of the site editor: /site-editor.php and then we're using queries for everything else, e.g. Pages: site-editor.php?p=%2Fpage, Styles: site-editor.php?p=%2Fstyles.
I wouldn't consider the site-editor.php as top level route, It's just a temporary technical limitation while we iterate on admin redesign. In fact we already explored pretty permalinks in the admin and we know it's possible.
Instead consider p as the "path" and the rest as query. This is exactly how the router works btw.
There was a problem hiding this comment.
The way I think about routes is that I try to map these to navigation menu
Instead consider p as the "path" and the rest as query. This is exactly how the router works btw.
Thanks for sharing that. Make sense to me.
So it seems close to me to a "modifier" than a "submenu", so I'd probably use query: /styles?preview=style-book or something.
➕
There was a problem hiding this comment.
Ok I've updated to use a query instead.
tellthemachines
force-pushed
the
try/stylebook-path
branch
from
02a3b12 to
7e9bbce
Compare
|
Flaky tests detected in 7e9bbce. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12306733362
|
tellthemachines
force-pushed
the
try/stylebook-path
branch
from
b7dff57 to
72ba66d
Compare
| return ( | ||
| <Button | ||
| isPressed={ isStyleBookOpened } | ||
| icon={ seen } | ||
| label={ __( 'Style Book' ) } | ||
| onClick={ () => { | ||
| setIsStyleBookOpened( ! isStyleBookOpened ); | ||
| const updatedPath = ! isStyleBookOpened | ||
| ? path.replace( '/styles', stylebookPath ) | ||
| : path.replace( stylebookPath, '/styles' ); |
There was a problem hiding this comment.
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' );There was a problem hiding this comment.
Oooh maybe, let me try!
There was a problem hiding this comment.
Yes it worked 😄 thanks!
| 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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hmm that was merged in the classic stylebook PR though I'm not seeing the same error on trunk. I'll investigate.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…ress#67811) * Give style book its own route so it can be linked to directly. * Fix paths to and from global styles. * Use query instead of path * Fix path * Effect for editor settings update
What?
Allow direct linking to style book:
Stylebookcomponent withStyleBookPreviewwhich renders the stylebook without the editor.Testing Instructions
Activate a block theme, open the site editor and navigate to Styles. Toggle the stylebook on and see URL change.
Refresh the page and see that stylebook still appears and the "Style Book" button remains in its toggled on state.
/wp-admin/site-editor.php?p=%2Fstyles/wp-admin/site-editor.php?p=%2Fstyles-stylebook