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

Home link: Allow usage outside the navigation block #60558

Closed
wants to merge 3 commits into from
Closed

Home link: Allow usage outside the navigation block #60558

wants to merge 3 commits into from

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Apr 8, 2024 •
edited
Loading

What?

Allows the home link block outside the navigation block.

Partial for #49282
Closes #35926

Why?

It is a more convenient way for end-users to add links to the home page without needing to use a navigation block, site logo, or block bindings API.

How?

The PR:

  • Removes the parent block limitation from block.json.
  • Adds showSubmenuIcon to usesContext in block.json, because compared to for example colors, this attribute it always set on the navigation block, so, it can be used to determine if the Home link block is inside the navigation block or not.
  • Conditionally prints the block using <a> or <li> depending on where it is placed.

Testing Instructions

Test this in both a template or template part and in a post or page.

Add a navigation block with a home link block inside.
Add a standalone home link block.

Confirm that both copies of the block work correctly in the editors and front:

  • All block settings work.
  • The correct element is used depending on where the block is placed.
  • The correct class names are used (the stand alone block should not have the navigation item class).
  • Confirm that the aria-current="page" is still present on the block when it is placed on the home page.

Testing Instructions for Keyboard

Feedback needed:

What is the correct element to use when the block is not inside the navigation block?

Screenshots or screencast

@carolinan carolinan added [Type] Enhancement A suggestion for improvement. Needs Accessibility Feedback Need input from accessibility [Block] Home Link Affects the Home Link Block labels Apr 8, 2024
@carolinan carolinan marked this pull request as ready for review April 10, 2024 08:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 10, 2024 •
edited
Loading

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: carolinan <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: jordesign <[email protected]>
Co-authored-by: iandunn <[email protected]>

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

fabiankaegy
fabiankaegy reviewed Apr 10, 2024
View reviewed changes
packages/block-library/src/home-link/index.php
Comment on lines +154 to +162
// If the home link is inside a navigation block, it should be wrapped in a list item with additional attributes.
// Use the block context to determine if the home link is inside a navigation block.
$in_navigation = ! empty( $block->context );

if ( $in_navigation ) {
$item_markup = '<li %1$s><a class="wp-block-home-link__content wp-block-navigation-item__content" href="%3$s" rel="home"%4$s>%5$s</a></li>';
} else {
$item_markup = '<a %2$s href="%3$s" rel="home"%4$s>%5$s</a>';
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be better covered via the block_core_navigation_listable_blocks filter introduced in 6.5.

Or you can add the block to the default array here:

gutenberg/packages/block-library/src/navigation/index.php

Lines 23 to 26 in e4623e1

private static $needs_list_item_wrapper = array(
'core/site-title',
'core/site-logo',
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Thanks I will look those up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though how would I avoid it breaking on 6.3, 6.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm mostly thinking out loud, but this block would also need the current-menu-item class on the li.

@carolinan
Copy link
Contributor Author

carolinan commented Apr 12, 2024 •
edited
Loading

The challenge here is to not break backwards compatibility.

Here is an example of a home link inside the navigation block without this PR:

<li class="has-text-color has-accent-3-color has-background has-accent-background-color has-small-font-size wp-block-navigation-item current-menu-item wp-block-home-link has-x-large-font-size"><a class="wp-block-home-link__content wp-block-navigation-item__content" href="http://65.local" rel="home" aria-current="page">Home</a></li>

Note the duplicate font sizes. The small font size is set on the navigation block, and the large is set on the home link.

If I switch to using the $needs_list_item_wrapper, array in the navigation block, then large chunks of code, including all the context, can be removed from the home link, but that results in something like this:

<li class="wp-block-navigation-item current-menu-item"><a class="wp-block-home-link has-x-large-font-size" href="http://65.local" rel="home" aria-current="page">Home</a></li>

So existing third-party CSS used to style the home link will break.

@carolinan
Copy link
Contributor Author

I created a separate branch with the changes that I mentioned above, where the list item is removed from home-link index.php and the block is added to $needs_list_item_wrapper in the navigation block's index.php.

eb0e1e5

@carolinan carolinan requested review from fabiankaegy and Mamaduka May 7, 2024 05:43
@github-actions GitHub Actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ packages/block-library/src/home-link/index.php

@github-actions GitHub Actions
Copy link

Flaky tests detected in 8d97521.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9204828530
📝 Reported issues:

@carolinan carolinan closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@ryanwelcher ryanwelcher Awaiting requested review from ryanwelcher

@juanmaguitar juanmaguitar Awaiting requested review from juanmaguitar juanmaguitar is a code owner

@ndiego ndiego Awaiting requested review from ndiego ndiego is a code owner

@fabiankaegy fabiankaegy Awaiting requested review from fabiankaegy fabiankaegy is a code owner

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

Assignees
No one assigned
Labels
[Block] Home Link Affects the Home Link Block Needs Accessibility Feedback Need input from accessibility [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Home Link block doesn't work well outside of menus
2 participants
@carolinan @fabiankaegy