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

Align Submenu block and Nav Link block by including description and wrapping span #67198

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

prajapatisagar
Copy link
Contributor

Addresses part of #57099

What?

This PR fixes the missing <span class="wp-block-navigation-item__label"> element for navigation items with submenus.

Why?

The absence of the <span class="wp-block-navigation-item__label"> element in navigation items with submenus results in inconsistent markup, potentially impacting accessibility and styling.

How?

The PR adjusts the rendering logic for navigation items with submenus to include the <span class="wp-block-navigation-item__label">.

Testing Instructions

  1. Add a navigation block
  2. Add a navigation item to this block
  3. Add a second navigation item to this block
  4. Add a submenu item to the second navigation block
  5. Check the source code

Testing Instructions for Keyboard

Screenshots or screencast

wp-block-navigation-item__label

Copy link

github-actions bot commented Nov 21, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: prajapatisagar <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: carolinan <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@akasunil akasunil added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block labels Nov 21, 2024
@akasunil akasunil requested a review from getdave November 22, 2024 05:29
@MaggieCabrera
Copy link
Contributor

I understand the goal is to bring parity between the editor and the frontend but this is extra markup/classnames that we need to support in the future. Why do we need this span? Either in the frontend and the editor, or both.

@getdave
Copy link
Contributor

getdave commented Nov 22, 2024

We should bring both navigation-link and submenu blocks into line with each other. That means:

  • remove span from both
  • add span to submenu

@getdave
Copy link
Contributor

getdave commented Nov 27, 2024

@MaggieCabrera As I understand it, I think the span is added to the core/navigation-link block because there are also scenarios where a description also gets added within the anchor tag.

if ( ! empty( $attributes['description'] ) ) {
$html .= '<span class="wp-block-navigation-item__description">';
$html .= wp_kses_post( $attributes['description'] );
$html .= '</span>';
}

We don't support this on the submenu, which is likely because submenu came after Navigation Link and the two blocks have fallen out of sync as they rely on a lot of duplication.

#52505 requests that description is added to the submenu block. Therefore I think we should consider pivotting this PR to add the description. At that point the span makes a lot more sense. Do you agree @MaggieCabrera?

@prajapatisagar Would you be interested in adjusting this PR accordingly?

@prajapatisagar
Copy link
Contributor Author

Yes will update the PR accordingly.

@MaggieCabrera
Copy link
Contributor

#52505 requests that description is added to the submenu block. Therefore I think we should consider pivotting this PR to add the description. At that point the span makes a lot more sense. Do you agree @MaggieCabrera?

Thank you for doing the work to uncover the reasons behind the current state of the block! That makes much easier to judge what the direction should be. And what you suggest sounds reasonable to me.

@prajapatisagar
Copy link
Contributor Author

@getdave I have updated the feedback.

@carolinan
Copy link
Contributor

Could the documentation about the class names be updated too as part of this PR?
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/navigation/README.md

@prajapatisagar
Copy link
Contributor Author

@carolinan I have updated the documentation.

@getdave getdave changed the title Navigation item with sub menu is missing wp-block-navigation-item__label span Align Submenu block and Nav Link block by including description and wrapping span Nov 29, 2024
@getdave getdave added the [Block] Submenu Affects the Submenu Block - for submenus in navigation label Nov 29, 2024
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thanks for your work here @prajapatisagar 👍

When it comes to WordPress 6.8 I suggest we include a note in the miscellaneous dev notes about these markup changes.

@getdave getdave added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Nov 29, 2024
@getdave getdave merged commit 826c430 into WordPress:trunk Dec 3, 2024
68 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Dec 3, 2024
im3dabasia pushed a commit to im3dabasia/gutenberg that referenced this pull request Dec 4, 2024
…rapping span (WordPress#67198)

* Fix missing wp-block-navigation-item__label span

* Addressed feedback

* Updated README.md

Co-authored-by: prajapatisagar <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: carolinan <[email protected]>
michalczaplinski pushed a commit that referenced this pull request Dec 5, 2024
…rapping span (#67198)

* Fix missing wp-block-navigation-item__label span

* Addressed feedback

* Updated README.md

Co-authored-by: prajapatisagar <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: carolinan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Block] Submenu Affects the Submenu Block - for submenus in navigation Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants