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

Jump to bottom

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
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
22 changes: 15 additions & 7 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,18 @@ class WP_Theme_JSON_Gutenberg {
);

const VALID_SETTINGS = array(
'border' => 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,
'hasColorSettings' => null,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

I've just run into another block that also uses the justification and I think this can be converted to a block supports: #34872

),
'border' => array(
'customColor' => null,
'customRadius' => null,
'customStyle' => null,
'customWidth' => null,
),
'color' => array(
'color' => array(
'background' => null,
'custom' => null,
'customDuotone' => null,
Expand All @@ -95,19 +100,19 @@ class WP_Theme_JSON_Gutenberg {
'palette' => null,
'text' => null,
),
'custom' => null,
'layout' => array(
'custom' => null,
'layout' => array(
'contentSize' => null,
'wideSize' => null,
),
'spacing' => array(
'spacing' => array(
'blockGap' => null,
'customMargin' => null,
'customPadding' => null,
'units' => null,
'blockGap' => null,
),
'typography' => array(
'typography' => array(
'customFontSize' => null,
'customFontStyle' => null,
'customFontWeight' => null,
Expand Down Expand Up @@ -315,11 +320,14 @@ private static function sanitize( $input, $valid_block_names, $valid_element_nam
}
$schema_styles_blocks = array();
$schema_settings_blocks = array();

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$schema_styles_blocks[ $block ] = self::VALID_STYLES;
$schema_styles_blocks[ $block ]['elements'] = $schema_styles_elements;
}


$schema['styles'] = self::VALID_STYLES;
$schema['styles']['blocks'] = $schema_styles_blocks;
$schema['styles']['elements'] = $schema_styles_elements;
Expand Down
6 changes: 5 additions & 1 deletion lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,14 +412,18 @@ public static function get_merged_data( $settings = array(), $origin = 'user' )

if (
! get_theme_support( 'experimental-link-color' ) && // link color support needs the presets CSS variables regardless of the presence of theme.json file.
! WP_Theme_JSON_Resolver_Gutenberg::theme_has_support()
! self::theme_has_support()
) {
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() ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


$result->merge( $filter_data );

if ( 'user' === $origin ) {
$result->merge( self::get_user_data() );
}
Expand Down
34 changes: 33 additions & 1 deletion lib/navigation-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,43 @@ function gutenberg_navigation_init( $hook ) {
* @return bool Filtered decision about loading block assets.
*/
function gutenberg_navigation_editor_load_block_editor_scripts_and_styles( $is_block_editor_screen ) {
if ( is_callable( 'get_current_screen' ) && get_current_screen() && 'gutenberg_page_gutenberg-navigation' === get_current_screen()->base ) {
if ( is_navigation_editor_screen() ) {
return true;
}

return $is_block_editor_screen;
}

add_filter( 'should_load_block_editor_scripts_and_styles', 'gutenberg_navigation_editor_load_block_editor_scripts_and_styles' );

/**
* Filters the Theme JSON to alter the settings of the Navigation block.
*
* @param Array $data the raw Theme JSON config at the Theme level.
* @return Array the amended Theme JSON config.
*/
function gutenberg_navigation_editor_filter_navigation_block_settings( $data ) {
if ( is_navigation_editor_screen() ) {
$data['settings']['blocks']['core/navigation'] = array(
'hasSubmenuIndicatorSetting' => false,
'hasItemJustificationControls' => false,
'hasColorSettings' => false,
Comment on lines +95 to +97
Copy link
Contributor Author

@getdave getdave Sep 13, 2021 •
edited by draganescu
Loading

Choose a reason for hiding this comment

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

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.

);
}

return $data;
}

add_filter(
'theme_json_resolver_merged_data',
'gutenberg_navigation_editor_filter_navigation_block_settings'
);

/**
* Determines whether we are on the navigation editor screen.
*
* @return boolean whether or not we are on the navigation editor screen.
*/
function is_navigation_editor_screen() {
return is_callable( 'get_current_screen' ) && get_current_screen() && 'gutenberg_page_gutenberg-navigation' === get_current_screen()->base;
}
2 changes: 2 additions & 0 deletions lib/navigation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,3 +384,5 @@ function gutenberg_add_block_menu_item_styles_to_nav_menus( $hook ) {
}
}
add_action( 'admin_enqueue_scripts', 'gutenberg_add_block_menu_item_styles_to_nav_menus' );


11 changes: 8 additions & 3 deletions packages/block-library/src/navigation/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
PanelColorSettings,
ContrastChecker,
getColorClassName,
useSetting,
} from '@wordpress/block-editor';
import { useDispatch, withSelect, withDispatch } from '@wordpress/data';
import { PanelBody, ToggleControl, ToolbarGroup } from '@wordpress/components';
Expand Down Expand Up @@ -103,9 +104,6 @@ function Navigation( {

// These props are used by the navigation editor to override specific
// navigation block settings.
hasSubmenuIndicatorSetting = true,
hasItemJustificationControls = true,
hasColorSettings = true,
customPlaceholder: CustomPlaceholder = null,
customAppender: CustomAppender = null,
} ) {
Expand All @@ -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.
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 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.


const hasItemJustificationControls =
useSetting( 'hasItemJustificationControls' ) ?? true; // retain original prop default of "true" if there is no setting defined.

const hasColorSettings = useSetting( 'hasColorSettings' ) ?? true; // retain original prop default of "true" if there is no setting defined.

const { selectBlock } = useDispatch( blockEditorStore );

Expand Down
9 changes: 1 addition & 8 deletions packages/edit-navigation/src/filters/remove-edit-unsupported-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,7 @@ const removeNavigationBlockEditUnsupportedFeatures = createHigherOrderComponent(
return <BlockEdit { ...props } />;
}

return (
<BlockEdit
{ ...props }
hasSubmenuIndicatorSetting={ false }
hasItemJustificationControls={ false }
hasColorSettings={ false }
/>
);
return <BlockEdit { ...props } />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look ma, no props! 🥳

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! It should be possible to delete the whole file and remove the lines where removeNavigationBlockEditUnsupportedFeatures is imported and called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤞

},
'removeNavigationBlockEditUnsupportedFeatures'
);
Expand Down