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

Experiment with allowing Theme JSON to control Navigation block within the Navigation Editor screen #34784

Closed
wants to merge 9 commits into from
+64 −20
Closed

Experiment with allowing Theme JSON to control Navigation block within the Navigation Editor screen #34784

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
83e6ded
Allow filtering of Theme JSON final config
getdave Sep 13, 2021
53100bc
Allow block-level settings to be added to valid settings config
getdave Sep 13, 2021
5b0a576
Allow for filtering of Theme-level theme JSON settings.
getdave Sep 13, 2021
4a96cdb
Get setting from modified Theme JSON rather than block level prop
getdave Sep 13, 2021
5a24c37
Default to previous prop value default only if setting is nullish
getdave Sep 13, 2021
06db44a
Allow Theme JSON to manage justification controls
getdave Sep 13, 2021
9ae1bf2
Add Theme JSON filter to main navigation file
getdave Sep 13, 2021
31a2dac
Allow color settings to be controlled via Theme JSON
getdave Sep 13, 2021
4aff550
Conditionalise filtering of Theme JSON to Navigation editor screen
getdave Sep 13, 2021
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
Prev Previous commit
Next Next commit
Allow Theme JSON to manage justification controls
  • Loading branch information
@getdave
getdave committed Sep 13, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
commit 06db44a857091f6dffad965eea0e001b8a9c0d9f
1 change: 1 addition & 0 deletions lib/class-wp-theme-json-gutenberg.php
View file
Original file line number Diff line number Diff line change
@@ -80,6 +80,7 @@ class WP_Theme_JSON_Gutenberg {
const VALID_SETTINGS = array(
'core/navigation' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@getdave getdave Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

'hasSubmenuIndicatorSetting' => null,
'hasItemJustificationControls' => null,
),
'border' => array(
'customColor' => null,