Experiment with allowing Theme JSON to control Navigation block within the Navigation Editor screen #34784
Conversation
getdave
added
the
Global Styles
getdave
added
[Block] Navigation
getdave
requested review from
adamziel,
ajitbohra,
kevin940726,
spacedmonkey,
tellthemachines and
TimothyBJacobs
as code owners
|
Size Change: +6 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
| foreach ( $valid_block_names as $block ) { | ||
| $schema_settings_blocks[ $block ] = self::VALID_SETTINGS; | ||
| $schema_settings_blocks[ $block ] = self::VALID_SETTINGS[ $block ] ? array_merge( self::VALID_SETTINGS, self::VALID_SETTINGS[ $block ] ) : self::VALID_SETTINGS; |
There was a problem hiding this comment.
Here if we have block level settings we merge with the top level settings to preserve backwards compat but allow extension at a block level.
| ) { | ||
| return $result; | ||
| } | ||
|
|
||
| $theme_support_data = WP_Theme_JSON_Gutenberg::get_from_editor_settings( $settings ); | ||
| $result->merge( self::get_theme_data( $theme_support_data ) ); | ||
|
|
||
| $filter_data = new WP_Theme_JSON_Gutenberg( apply_filters( 'theme_json_resolver_merged_data', $result->get_raw_data() ) ); |
There was a problem hiding this comment.
$result is an instance of WP_Theme_JSON_Gutenberg so I'm passing in the raw data from the current instance and allowing it to be filtered and then creating a new instance of WP_Theme_JSON_Gutenberg. Then I can call merge on it to merge in the data coming from the filter.
There was a problem hiding this comment.
I'd avoid adding filters as of now, as per https://github.com/WordPress/gutenberg/pull/34784/files#r709896696 but also this and this may be good context.
#34843 is promising. If we decide to go for something like that, I see the filters belonging to those functions instead of this inner classes.
| 'hasSubmenuIndicatorSetting' => false, | ||
| 'hasItemJustificationControls' => false, | ||
| 'hasColorSettings' => false, |
There was a problem hiding this comment.
These x3 settings were originally props on the Navigation block. We had to use filters to override them.
The new approach allow us to toggle these via Theme JSON.
| @@ -115,6 +113,13 @@ function Navigation( { | |||
| const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] = useState( | |||
| false | |||
| ); | |||
| const hasSubmenuIndicatorSetting = | |||
| useSetting( 'hasSubmenuIndicatorSetting' ) ?? true; // retain original prop default of "true" if there is no setting defined. | |||
There was a problem hiding this comment.
The benefit of this approach is that useSetting is much more declarative. If the a setting is add/removed/changed then it's much easier for the Nav Editor to declare support (or not!).
Also it's much harder for developers working on the Nav block to accidentally break the Nav Editor because making a change to a setting is a larger undertaking.
| hasColorSettings={ false } | ||
| /> | ||
| ); | ||
| return <BlockEdit { ...props } />; |
There was a problem hiding this comment.
Look ma, no props! 🥳
There was a problem hiding this comment.
Nice! It should be possible to delete the whole file and remove the lines where removeNavigationBlockEditUnsupportedFeatures is imported and called.
| @@ -78,13 +78,18 @@ class WP_Theme_JSON_Gutenberg { | |||
| ); | |||
|
|
|||
| const VALID_SETTINGS = array( | |||
| 'border' => array( | |||
| 'core/navigation' => array( | |||
There was a problem hiding this comment.
I worry that if all blocks can add things here:
- we'll get a huge array in the end, these valid bits should be only for general settings/styles IMO
- how will 3rd part blocks use this possibility?
Can't the block register its own settings?
There was a problem hiding this comment.
Thanks for taking a look.
these valid bits should be only for general settings/styles IMO
Anything that isn't "valid" isn't allowed in the Theme JSON config. Therefore if we don't allow blocks to add to this we can't control them via Theme JSON at this level of granularity.
If the consensus is that Theme JSON should only allow for configuration that applies to all blocks, then the feature has limited somewhat reduced utility for the configuration of (more specialised) blocks as it's a fact that many (almost all) block features won't apply generally across the board.
The objectives that this exploration seeks to explore are:
- we should be able to easily and reliably re-configure the Nav block to be simpler for use in the Nav Editor screen in such a way as it will be resilient to change.
- Themers should be able to easily configure the Nav block to support feature X or Y (outside of the context of the Nav Editor).
As a result, I think we should at least consider opening up Theme JSON schema for extension by developers (Core or otherwise).
By doing so we will afford great flexibility and power to block builders to allow Themers (or even users!) to configure their blocks without needing to understanding complex hook/filters.
Key point: if we don't agree that Theme JSON should be extensible, then it would seem that we will need to declare it as not suitable for configuring blocks at a granular level. It will be purely for configuring common things such as spacing, colors...etc, but not for individual features.
The knock on effect for this on the Nav Editor project will be that we must move Theme JSON alongside Block Supports API as defunct for the purposes of configuring the Nav block inside the Nav Editor. This will leave us back where we are today - a plethora of brittle hooks and filters and overrides.
I've had little/limited involvement in the Theme JSON project and so I'd like to ask the opinion of @oandregal (and others) what they think. If this experiment is going against core principles of Theme JSON and Global Styles then please do let us know.
we'll get a huge array in the end
This is also a concern of mine.
how will 3rd part blocks use this possibility
Good question. They can't at the moment. However, we could (in theory) expose APIs to allow blocks to register their own Theme JSON settings.
Can't the block register its own settings?
It can't right now because this is a PoC 😄 But in the future we could enable this.
There was a problem hiding this comment.
Could we use the block definition (block.json) to register block-specific settings? Perhaps a new supports.custom settings section? Or perhaps we allow providing data for any registered attribute of the block?
My overall thinking is that if we want to do this, the first step is to figure out a way that works for any block, whether it's a core block or a 3rd party block. As a general approach, I'd explore a path that doesn't involve letting others filter the theme.json tree directly, otherwise if/when we want to change the theme.json shape we won't be able to do it without breaking the users of those filters. If we use a declarative approach (like pulling data from block.json to the proper places) we are in a better spot to modify the theme.json tree in the future.
There was a problem hiding this comment.
we should be able to easily and reliably re-configure the Nav block to be simpler for use in the Nav Editor screen in such a way as it will be resilient to change.
I'm largely unfamiliar with the inner workings of the navigation block, so I may have misunderstood things or lack the proper context. But just wanted to re-share that both block-supports and theme.json are editor-agnostic so far: they aren't meant to configure things per editor.
There was a problem hiding this comment.
they aren't meant to configure things per editor
Thanks for reiterating this. We're aware the approach would be...unusual.
What we're trying to do is explore all avenues so as to be sure we've ruled out all options before we decide on a preferred direction.
There was a problem hiding this comment.
The topic of allowing settings per-block is something that we've discussed in a few PRs and issues. I figured it'd be good to have a single place to share our thoughts on it, so I created #35114
| 'core/navigation' => array( | ||
| 'hasSubmenuIndicatorSetting' => null, | ||
| 'hasItemJustificationControls' => null, | ||
| 'hasColorSettings' => null, |
There was a problem hiding this comment.
There is a color array just below, I was wondering if it should be possible to override the color settings using that instead of a new setting?
There was a problem hiding this comment.
This makes me wonder if the hasColorSettings prop was wrong all along, and maybe the navigation block should instead use useSetting to determine whether the color settings panel is rendered.
There was a problem hiding this comment.
It's probably ok for us to use useSetting for that one in this case as it's a general UI config item which has now been made part of Global Styles.
There was a problem hiding this comment.
In addition to what Dan mentioned about hasColorSettings I wonder whether hasItemJustificationControls can be done the same way, see discussion about that at #34317 (the issue is unowned, feel free to take it).
There was a problem hiding this comment.
I've just run into another block that also uses the justification and I think this can be converted to a block supports: #34872
Questions requiring answersThis PR was shared in Core Editor chat. @jorgefilipecosta responded with his thoughts on whether it is a good route to continue to explore. Key points from Jorge
Questions requiring answersI will try to answer the following questions within this PR or in an associated Issue:
|
|
👋 I've left a few practical comments about this: how to make this work for any block?, have we considered using the existing settings and/or expand them to do this?, and that I don't think we should add this filter. Some high-level thoughts:
|
I agree, I think ideally those two things use block supports. That would be a really nice improvement generally for the block. |
|
Thank you for all the input here. I do appreciate it 🙇 In general I'd like to refer folks back to the previous exploration we did on using Block Supports API to "configure" the Nav block. One of the takeaways was
Given this, I'm curious....how would moving some of the block's "settings" (ie: colour) over to Block Supports help the Nav block to be configured differently in the Nav editor? |
@getdave Oh, looking at the block.json there's more to this than I originally thought. The navigation block previously used the color block supports option, but it looks like it was removed (here: https://github.com/WordPress/gutenberg/pull/31149/files#diff-554049ef85f1660a9ba54b7ee492d7d347a5244a9aa4a7d3fb75ab24015c2f29L71). I didn't realise that had happened, so in my comments I wasn't really considering this as a move to block supports, more as a tidy-up. |
|
@oandregal What I discovered in my previous comment might mean that the theme.json color options for the nav block no longer work. Might be worth checking this! |
Yeah, blocks that do not use the block supports aren't supposed to work via |
As I see it, this PR is trying to add code so the |
|
I just want to chime in to say that what we're after is something like "runtime custom block configurability", that is the ability for blocks to declare arbitrary configuration that can be set up when the block is programmatically used. So far all exploration hit the fact that all block configuration is not custom to the block, but generic configuration we want all blocks to have access to. But take the "show submenus arrow" navigation block setting. We want to have a way in which the navigation block when used in the navigation editor has this setting unavailable (not set to false, unavailable). Also a theme might want the same thing, because the theme for menu M may recommend depth 1 menus for whatever reason. Nevertheless, if we don't want any support for such block configuration then the only path forward that makes sense for the editor is to support a "classic menu block" instead of constantly overriding the navigation block. At the same time, in my mind, the ability to programmatically alter block behavior is highly valuable and needed by themes and plugins. |
|
Thanks for clarifying this Andrei 👍 I came to much the same conclusion on the canonical Issue for which this Theme JSON PR was an exploration. Closing this out as it's unlikely we're going to be pursuing it. |
Description
As part of #30007 (comment) I proposed we could control some of the features of the Navigation block via Theme JSON. This would:
This is a PoC PR which experiments with this approach for x3 Nav block settings for which were currently rely on controlling via
propsand modifying (for the Nav Editor) using theeditor.BlockEditfilter.To do this I have
settings.blockskey. These would likely be custom to the block and allows any block to define its own extension point.false.editimplementation to useuseSetting()to get the block's behaviour configuration data (settings) from Theme JSON rather than from props.The result should be exactly the same as what we have in
trunkright now. The difference is we're controlling via Theme JSON and a filter.Closes #34775
How has this been tested?
gutenberg_navigation_editor_filter_navigation_block_settingsfunction insidelib/navigation-page.phpand try turning the features on and off. They should reflect in the Nav Editor only.true- this mirrors the original default settings for thepropversions of the settings.theme.jsonfile to configure the Nav block outside of the Nav editor.Screenshots
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist:
*.native.jsfiles for terms that need renaming or removal).