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

Change Navigation block markup on front end only #30551

Merged
merged 6 commits into from
Jul 7, 2021
Merged

Change Navigation block markup on front end only #30551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eeb888f
Change markup on front end
tellthemachines Apr 7, 2021
9661375
Fix rendering issue with responsive wrapper
tellthemachines May 28, 2021
2ecb7a6
Use divs in Nav link; remove Spacer from lists
tellthemachines Jun 24, 2021
221bdca
lint php
tellthemachines Jun 24, 2021
877162b
Update e2e test selectors
tellthemachines Jun 28, 2021
3e7d0fb
Update Home Link semantics
tellthemachines Jun 29, 2021
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
4 changes: 2 additions & 2 deletions packages/block-library/src/home-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export default function HomeEdit( {

return (
<>
<li { ...blockProps }>
<div { ...blockProps }>
<a
className="wp-block-home-link__content"
href={ homeUrl }
Expand All @@ -80,7 +80,7 @@ export default function HomeEdit( {
] }
/>
</a>
</li>
</div>
</>
);
}
9 changes: 4 additions & 5 deletions packages/block-library/src/navigation-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ import { store as coreStore } from '@wordpress/core-data';
import { ItemSubmenuIcon } from './icons';
import { name } from './block.json';

const ALLOWED_BLOCKS = [ 'core/navigation-link', 'core/spacer' ];
const ALLOWED_BLOCKS = [ 'core/navigation-link' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we mean to remove the spacer from being insertable from submenus? We could maybe leave it for a follow up, it might be possible to do something like modify padding on the items instead of inserting an element for the published view.

Screen Shot 2021-06-25 at 10 46 07 AM

As a note for the other reviewers, existing menus with a spacer still appear, but new menus won't allow spacers to be inserted in sub-menus.

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 removed it temporarily, as there are a couple of things we need to fix for it to work properly in submenus:

  • the markup issue (ideally by getting the spacer to not render markup at all, or else by wrapping it in an li)
  • making the spacer vertical within the submenu of a horizontal nav (right now it only works vertically within vertical nav blocks)

Copy link
Contributor Author

@tellthemachines tellthemachines Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Update: I created #33018 to discuss the best way forward for the Spacer markup issue, and there is #30590 for its behaviour in horizontal Nav submenus. I think we can fix these separately from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some "phase 2" of the block, we'll want to tackle mega-menus, where the contents of a dropdown are virtually as flexible as what you can insert in a group block, so it's something we should come back to. But definitely fine to wrap a solid V1 first.


const MAX_NESTING = 5;

Expand Down Expand Up @@ -418,7 +418,6 @@ export default function NavigationLinkEdit( {
hasDescendants
? InnerBlocks.DefaultAppender
: false,
__experimentalAppenderTagName: 'li',
}
);

Expand Down Expand Up @@ -507,7 +506,7 @@ export default function NavigationLinkEdit( {
/>
</PanelBody>
</InspectorControls>
<li { ...blockProps }>
<div { ...blockProps }>
{ /* eslint-disable jsx-a11y/anchor-is-valid */ }
<a className={ classes }>
{ /* eslint-enable */ }
Expand Down Expand Up @@ -611,8 +610,8 @@ export default function NavigationLinkEdit( {
</span>
) }
</a>
<ul { ...innerBlocksProps } />
</li>
<div { ...innerBlocksProps } />
</div>
</Fragment>
);
}
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ function Navigation( {
isOpen={ isResponsiveMenuOpen }
isResponsive={ attributes.isResponsive }
>
<ul { ...innerBlocksProps }></ul>
<div { ...innerBlocksProps }></div>
</ResponsiveWrapper>
</nav>
</>
Expand Down
13 changes: 11 additions & 2 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,16 @@ function render_block_core_navigation( $attributes, $content, $block ) {
);

$inner_blocks_html = '';
$is_list_open = false;
foreach ( $block->inner_blocks as $inner_block ) {
if ( ( 'core/navigation-link' === $inner_block->name || 'core/home-link' === $inner_block->name ) && false === $is_list_open ) {
$is_list_open = true;
$inner_blocks_html .= '<ul class="wp-block-navigation__container">';
}
if ( 'core/navigation-link' !== $inner_block->name && 'core/home-link' !== $inner_block->name && true === $is_list_open ) {
$is_list_open = false;
$inner_blocks_html .= '</ul>';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, much closer to expected html markup. If we go with this approach, children of ul, still need to be wrapped in an li. (For example the spacer div).

Screen Shot 2021-04-26 at 8 59 14 AM

$inner_blocks_html .= $inner_block->render();
}

Expand All @@ -193,7 +202,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {
// return early if they don't.
if ( ! isset( $attributes['isResponsive'] ) || false === $attributes['isResponsive'] ) {
return sprintf(
'<nav %1$s><ul class="wp-block-navigation__container">%2$s</ul></nav>',
'<nav %1$s>%2$s</nav>',
$wrapper_attributes,
$inner_blocks_html
);
Expand All @@ -206,7 +215,7 @@ function render_block_core_navigation( $attributes, $content, $block ) {
<div class="wp-block-navigation__responsive-dialog" role="dialog" aria-modal="true" aria-labelledby="modal-%1$s-title" >
<button aria-label="%4$s" data-micromodal-close class="wp-block-navigation__responsive-container-close"><svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-hidden="true" focusable="false"><path d="M13 11.8l6.1-6.3-1-1-6.1 6.2-6.1-6.2-1 1 6.1 6.3-6.5 6.7 1 1 6.5-6.6 6.5 6.6 1-1z"></path></svg></button>
<div class="wp-block-navigation__responsive-container-content" id="modal-%1$s-content">
<ul class="wp-block-navigation__container">%2$s</ul>
%2$s
</div>
</div>
</div>
Expand Down
11 changes: 11 additions & 0 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@
.wp-block-navigation {
position: relative;

// Horizontal layout
display: flex;
flex-wrap: wrap;
align-items: center;

// Vertical layout
&.is-vertical {
flex-direction: column;
align-items: flex-start;
}

// Normalize list styles.
ul,
ul li {
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/experiments/blocks/navigation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ describe( 'Navigation', () => {
// Scope element selector to the Editor's "Content" region as otherwise it picks up on
// block previews.
const navBlockItemsLength = await page.$$eval(
'[aria-label="Editor content"][role="region"] li[aria-label="Block: Custom Link"]',
'[aria-label="Editor content"][role="region"] div[aria-label="Block: Custom Link"]',
( els ) => els.length
);

Expand Down
4 changes: 2 additions & 2 deletions packages/e2e-tests/specs/experiments/navigation-editor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ describe( 'Navigation editor', () => {

// Select a link block with nested links in a submenu.
const parentLinkXPath =
'//li[@aria-label="Block: Custom Link" and contains(.,"WordPress.org")]';
'//div[@aria-label="Block: Custom Link" and contains(.,"WordPress.org")]';
const linkBlock = await page.waitForXPath( parentLinkXPath );
await linkBlock.click();

Expand All @@ -322,7 +322,7 @@ describe( 'Navigation editor', () => {
// Submenus are hidden using `visibility: hidden` and shown using
// `visibility: visible` so the visible/hidden options must be used
// when selecting the elements.
const submenuLinkXPath = `${ parentLinkXPath }//li[@aria-label="Block: Custom Link"]`;
const submenuLinkXPath = `${ parentLinkXPath }//div[@aria-label="Block: Custom Link"]`;
const submenuLinkVisible = await page.waitForXPath( submenuLinkXPath, {
visible: true,
} );
Expand Down