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

Use correct classnames for navigation link block save output #18926

Merged
merged 2 commits into from Dec 6, 2019

Conversation

@talldan
Copy link
Contributor

talldan commented Dec 5, 2019

Description

This PR tidies up some of the classnames in the Navigation Link block:

  • Fixes the incorrectly named wp-block-navigation-item class to be wp-block-navigation-link.
  • Renames wp-block-navigation-item__link to wp-block-navigation-link__content, which matches the classname used in the block edit for the link text.
  • Removes the unused wp-block-navigation-item__inner class from the block edit.

This doesn't affect any styles as the block's save output is styled using element selectors.

Fixes #18914.

How has this been tested?

  1. Add a Navigation block
  2. Add multiple inner Navigation Link blocks
  3. Inspect the editor html in the dev tools.
  4. Observe that the class wp-block-navigation-link__inner no longer appears
  5. Preview the post and inspect the post html
  6. Expect that the correct classnames appear (wp-block-navigation-link and wp-block-navigation-link__content).
  7. Expect that the block still appears the same visually.

Types of changes

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

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
Copy link
Member

noisysocks left a comment

Thanks Dan!

@@ -208,7 +208,7 @@ function NavigationLinkEdit( {
'has-link': !! url,
} ) }
>
<div className="wp-block-navigation-link__inner">
<div>

This comment has been minimized.

Copy link
@noisysocks

noisysocks Dec 5, 2019

Member

Can we get rid of this <div>?

This comment has been minimized.

Copy link
@talldan

talldan Dec 6, 2019

Author Contributor

I don't think so as it's what the LinkControl Popover uses to position itself.

@@ -77,12 +77,12 @@ function render_block_navigation( $attributes, $content, $block ) {
function build_navigation_html( $block, $colors ) {
$html = '';
$css_classes = implode( ' ', $colors['css_classes'] );
$class_attribute = sprintf( ' class="%s"', esc_attr( ! empty( $css_classes ) ? 'wp-block-navigation-item__link ' . $css_classes : 'wp-block-navigation-item__link' ) );
$class_attribute = sprintf( ' class="wp-block-navigation-link__content %s"', esc_attr( trim( $css_classes ) ) );

This comment has been minimized.

Copy link
@noisysocks

noisysocks Dec 5, 2019

Member

Seeing this kind of code makes me want to add wp_classnames() to Core 😀

@talldan talldan merged commit 2d2c368 into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@talldan talldan deleted the fix/classnames-for-nav-links branch Dec 6, 2019
$style_attribute = $colors['inline_styles'] ? sprintf( ' style="%s"', esc_attr( $colors['inline_styles'] ) ) : '';

foreach ( (array) $block['innerBlocks'] as $key => $block ) {

$html .= '<li class="wp-block-navigation-item">' .
$html .= '<li class="wp-block-navigation-link">' .

This comment has been minimized.

Copy link
@retrofox

retrofox Dec 7, 2019

Contributor

I think we should consider there is only one type of item available for the <Navigation /> which is the <NavigationLink />, but it's possible in the near future that we will have more items, such as media items, a simple paragraph, etc.
For this, the name link should be applied only in the navigation item block, not in the navigation one, imo.

This comment has been minimized.

Copy link
@talldan

talldan Dec 9, 2019

Author Contributor

@retrofox, the way the navigation block works is different to other blocks. The inner Navigation Link blocks are rendered server side by Navigation Block. This is the code that does that, so each <li/> here is a Navigation Link.

Maybe there's some way to improve this in the future so that the class name isn't defined in the parent block, but for now the main thing is that the editor and post class names match up as much as possible for themers.

This comment has been minimized.

Copy link
@retrofox

retrofox Dec 9, 2019

Contributor

yes, I forgot and got confused a little bit. But my point is that the class name shouldn't be link for all items. I think we are completely ok so far since there is only one kind of item which is the link, but we should keep in mind that the idea is maybe to get more items. In fact, we could deduce the kind of item through the block attributes.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
…ss#18926)

* Use correct classnames for navigation link block save output

* Classname audit. Remove unused `__inner` classname. Rename `__link` classname to `__content` to match the block edit classnames
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.