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.

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

Navigation: Allow additional CSS classes #18466

Merged
merged 6 commits into from Nov 15, 2019
Merged

Navigation: Allow additional CSS classes #18466

merged 6 commits into from Nov 15, 2019

Conversation

@obenland
Copy link
Member

obenland commented Nov 12, 2019

Fixes a bug where additional CSS classes were not passed to the front-end.

How has this been tested?

When creating a navigation menu, add a CSS class in block settings.
Preview the block and make sure the class gets applied to the nav element.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
@obenland obenland requested review from retrofox and draganescu Nov 12, 2019
@obenland obenland self-assigned this Nov 12, 2019
@obenland obenland added this to 👀 PRs to review in Navigation block via automation Nov 12, 2019
Copy link
Contributor

draganescu left a comment

Thanks! This is great, since adding the classname was there in the editor but not rendering it made it useless /o\

In my testing adding a css class now results in it being rendered, but it also adds an empty style attribute to the nav root element.

Screenshot 2019-11-13 at 12 32 07

@obenland

This comment has been minimized.

Copy link
Member Author

obenland commented Nov 13, 2019

It'll be empty if there are no custom colors selected and shouldn't have any detrimental effect on display or performance of the block.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 14, 2019

Can't we not have it? Other blocks seem to manage this very well and avoid empty attributes at all times in what I have seen so far :)

@obenland obenland requested a review from draganescu Nov 14, 2019
obenland added 3 commits Nov 12, 2019
Fixes a bug where additional CSS classes were not passed to the front-end.
@obenland obenland force-pushed the fix/additional-class branch from 211d52b to e8dcabc Nov 14, 2019
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 15, 2019

Glad to see this being fixed. I was adding in some style variations and found this issue.

$colors = build_css_colors( $attributes );
$class_attribute = sprintf( ' class="%s"', esc_attr( $colors['css_classes'] ? 'wp-block-navigation-menu ' . $colors['css_classes'] : 'wp-block-navigation-menu' ) );
$style_attribute = $colors['inline_styles'] ? sprintf( ' style="%s"', esc_attr( $colors['inline_styles'] ) ) : '';
$colors = build_css_colors( $attributes );

This comment has been minimized.

Copy link
@aduth

aduth Nov 15, 2019

Member

Is this function build_css_colors not intended to be namespaced / prefixed specific to the plugin / block / WordPress? (i.e. is it meant to be for general usage?)

This comment has been minimized.

Copy link
@obenland

obenland Nov 15, 2019

Author Member

Is there a namespacing standard for PHP functions in Gutenberg?

This comment has been minimized.

Copy link
@aduth

aduth Nov 15, 2019

Member

Is there a namespacing standard for PHP functions in Gutenberg?

Now that you mention it, not explicitly.

For most functions, we follow best practices in using a gutenberg_ prefix for plugin-specific functions.

It's a little trickier for these blocks though, since they're copied verbatim into core. So the concern is in considering that, should this be merged to core, these become new global WordPress functions.

Most other dynamic blocks follow some convention of render_block_foo and register_block_foo, but there's definitely some (unfortunate) ad hoc naming for similar utility functions (example).

It's definitely not a blocker for merging this pull request, since it wasn't introduced here, but we should see about establishing a convention here that makes sense.

This comment has been minimized.

Copy link
@obenland

obenland Nov 15, 2019

Author Member

namespace Gutenberg; 😱

@obenland obenland merged commit 5d4a752 into master Nov 15, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 15, 2019
@obenland obenland deleted the fix/additional-class branch Nov 15, 2019
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.