Skip to content

Navigation Submenu: Refactor to use the block supports function #48936

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

Merged
scruffian merged 10 commits into from
Mar 23, 2023
+241 −75
Merged

Navigation Submenu: Refactor to use the block supports function #48936

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
17f38a2
Navigation Submenu: Remove needed code
scruffian Mar 8, 2023
996abf1
php lint
scruffian Mar 9, 2023
68526bd
Create test scaffold from Nav Link block tests
getdave Mar 21, 2023
44fd29c
Scaffold out first colors test
getdave Mar 21, 2023
02b199b
Assert color applied from custom colors
getdave Mar 21, 2023
99bc885
Validate classes not applied if not present
getdave Mar 21, 2023
bb098ed
Test for mixed custom and presets
getdave Mar 21, 2023
4807578
Experiment with utilising WP_HTML_Tag_Processor to improve tests
getdave Mar 21, 2023
168a906
Remove unused code
getdave Mar 21, 2023
67f9d9c
rename variables
scruffian Mar 22, 2023
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
php lint
  • Loading branch information
@scruffian
scruffian committed Mar 9, 2023
commit 996abf10191e029c2ee782602bfe6cd7bdc8bc42
5 changes: 2 additions & 3 deletions packages/block-library/src/navigation-submenu/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,8 @@ function render_block_core_navigation_submenu( $attributes, $content, $block ) {

// This allows us to be able to get a response from gutenberg_apply_colors_support.
$block->block_type->supports['color'] = true;
Copy link
Contributor Author

@scruffian scruffian Mar 8, 2023

Choose a reason for hiding this comment

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

Is it ok to modify this?

Copy link
Contributor

@getdave getdave Mar 21, 2023

Choose a reason for hiding this comment

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

Why not rely on the actual block supports. If someone decides to disable block supports for this block then we should respect that.

Copy link
Contributor Author

@scruffian scruffian Mar 22, 2023

Choose a reason for hiding this comment

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

If we do that then users will be able to modify the settings for the submenu block in the UI, independently from the parent block. I think what we need to do is move the color settings from the navigation block to the submenu block, which will require a UI update and a block transform, and a lot of thinking about backwards compatibility. I do think we should do all of that, but if I had done it in this PR you'd be telling me it was too big, so this is just a first step toward that goal.

Copy link
Contributor

@getdave getdave Mar 22, 2023

Choose a reason for hiding this comment

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

Ok sure. I agree with small PRs.

I was thinking that as $block->block_type->supports['color'] = true; appears to indicate the supports is a Boolean we could just rely on that Boolean being set in the block.json for Submenu. But now I think I understand that if you did that the UI for colors would appear in the Submenu block - which isn't what we want.

So yes I think this workaround is an ok tradeoff for being able to utilise the standardised color code generation.

What's the long term plan? Expose UI in the Submenu Block to allow changing colors on individual blocks but have the initial settings still coming from the parent Nav block?

Copy link
Contributor Author

@scruffian scruffian Mar 22, 2023

Choose a reason for hiding this comment

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

What's the long term plan? Expose UI in the Submenu Block to allow changing colors on individual blocks but have the initial settings still coming from the parent Nav block?

I agree that we still need a way to set it on the Nav block so that you don't need to change it for every child, but I've not given much thought to how that would work in practice yet.

Copy link
Contributor

@draganescu draganescu Mar 23, 2023

Choose a reason for hiding this comment

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

I had the same gut reaction initially and even tried to code a variant that just relies on block supports, but we do have some blockers there. However:

users will be able to modify the settings for the submenu block in the UI, independently from the parent block

@jasmussen is that a problem really?

$colors_supports = gutenberg_apply_colors_support( $block->block_type, $attributes );

$css_classes = 'wp-block-navigation__submenu-container';
$colors_supports = gutenberg_apply_colors_support( $block->block_type, $attributes );
$css_classes = 'wp-block-navigation__submenu-container';
if ( array_key_exists( 'class', $colors_supports ) ) {
$css_classes .= ' ' . $colors_supports['class'];
}
Expand Down