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

Submenu: Add revert button. #38203

Merged
merged 21 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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
50 changes: 43 additions & 7 deletions packages/block-library/src/navigation-submenu/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ import {
createInterpolateElement,
} from '@wordpress/element';
import { placeCaretAtHorizontalEdge } from '@wordpress/dom';
import { link as linkIcon } from '@wordpress/icons';
import { link as linkIcon, removeSubmenu } from '@wordpress/icons';
import { store as coreStore } from '@wordpress/core-data';
import { speak } from '@wordpress/a11y';
import { createBlock } from '@wordpress/blocks';

getdave marked this conversation as resolved.
Show resolved Hide resolved
/**
* Internal dependencies
Expand Down Expand Up @@ -294,9 +295,11 @@ export default function NavigationSubmenuEdit( {
};
const { showSubmenuIcon, openSubmenusOnClick } = context;
const { saveEntityRecord } = useDispatch( coreStore );
const { __unstableMarkNextChangeAsNotPersistent } = useDispatch(
blockEditorStore
);

const {
__unstableMarkNextChangeAsNotPersistent,
replaceBlock,
} = useDispatch( blockEditorStore );
const [ isLinkOpen, setIsLinkOpen ] = useState( false );
const listItemRef = useRef( null );
const isDraggingWithin = useIsDraggingWithin( listItemRef );
Expand All @@ -312,20 +315,39 @@ export default function NavigationSubmenuEdit( {
selectedBlockHasDescendants,
userCanCreatePages,
userCanCreatePosts,
onlyDescendantIsEmptyLink,
} = useSelect(
( select ) => {
const {
getClientIdsOfDescendants,
hasSelectedInnerBlock,
getSelectedBlockClientId,
getBlockParentsByBlockName,
getBlock,
} = select( blockEditorStore );

let _onlyDescendantIsEmptyLink;

const selectedBlockId = getSelectedBlockClientId();

const descendants = getClientIdsOfDescendants( [ clientId ] )
.length;

const selectedBlockDescendants = getClientIdsOfDescendants( [
selectedBlockId,
] );

// Check for a single descendant in the submenu. If that block
// is a link block in a "placeholder" state with no URL then
// we can consider as an "empty" link.
if ( selectedBlockDescendants?.length === 1 ) {
const singleBlock = getBlock( selectedBlockDescendants[ 0 ] );

_onlyDescendantIsEmptyLink =
singleBlock?.name === 'core/navigation-link' &&
getdave marked this conversation as resolved.
Show resolved Hide resolved
! singleBlock?.attributes?.url;
getdave marked this conversation as resolved.
Show resolved Hide resolved
}

return {
isAtMaxNesting:
getBlockParentsByBlockName( clientId, name ).length >=
Expand All @@ -341,9 +363,7 @@ export default function NavigationSubmenuEdit( {
false
),
hasDescendants: !! descendants,
selectedBlockHasDescendants: !! getClientIdsOfDescendants( [
selectedBlockId,
] )?.length,
selectedBlockHasDescendants: !! selectedBlockDescendants?.length,
userCanCreatePages: select( coreStore ).canUser(
'create',
'pages'
Expand All @@ -352,6 +372,7 @@ export default function NavigationSubmenuEdit( {
'create',
'posts'
),
onlyDescendantIsEmptyLink: _onlyDescendantIsEmptyLink,
};
},
[ clientId ]
Expand Down Expand Up @@ -529,6 +550,11 @@ export default function NavigationSubmenuEdit( {

const ParentElement = openSubmenusOnClick ? 'button' : 'a';

function transformToLink() {
Copy link
Contributor

@adamziel adamziel Feb 9, 2022

Choose a reason for hiding this comment

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

wrapping this in useCallback could help avoid some re-rendering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you notice a performance problem here? If not I generally adhere to KCD.

const newLinkBlock = createBlock( 'core/navigation-link', attributes );
replaceBlock( clientId, newLinkBlock );
}

return (
<Fragment>
<BlockControls>
Expand All @@ -542,6 +568,16 @@ export default function NavigationSubmenuEdit( {
onClick={ () => setIsLinkOpen( true ) }
/>
) }
{ ( ! selectedBlockHasDescendants ||
onlyDescendantIsEmptyLink ) && (
<ToolbarButton
name="revert"
icon={ removeSubmenu }
title={ __( 'Convert to Link' ) }
onClick={ transformToLink }
className="wp-block-navigation__submenu__revert"
/>
) }
</ToolbarGroup>
</BlockControls>
<InspectorControls>
Expand Down
142 changes: 128 additions & 14 deletions packages/e2e-tests/specs/editor/blocks/navigation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
loginUser,
deleteUser,
switchUserToAdmin,
clickBlockToolbarButton,
} from '@wordpress/e2e-test-utils';
import { addQueryArgs } from '@wordpress/url';

Expand Down Expand Up @@ -121,6 +122,20 @@ async function selectClassicMenu( optionText ) {
await theOption.click();
}

async function populateNavWithOneItem() {
// Add a Link block first.
const appender = await page.waitForSelector(
'.wp-block-navigation .block-list-appender'
);
await appender.click();
// Add a link to the Link block.
await updateActiveNavigationLink( {
url: 'https://wordpress.org',
label: 'WP',
type: 'url',
} );
}

const PLACEHOLDER_ACTIONS_CLASS = 'wp-block-navigation-placeholder__actions';
const PLACEHOLDER_ACTIONS_XPATH = `//*[contains(@class, '${ PLACEHOLDER_ACTIONS_CLASS }')]`;
const START_EMPTY_XPATH = `${ PLACEHOLDER_ACTIONS_XPATH }//button[text()='Start empty']`;
Expand Down Expand Up @@ -194,6 +209,13 @@ async function getNavigationMenuRawContent() {
return stripPageIds( response.content.raw );
}

async function waitForBlock( blockName ) {
const blockSelector = `[aria-label="Editor content"][role="region"] [aria-label="Block: ${ blockName }"]`;

// Wait for a Submenu block before making assertion.
return page.waitForSelector( blockSelector );
}

// Disable reason - these tests are to be re-written.
// eslint-disable-next-line jest/no-disabled-tests
describe( 'Navigation', () => {
Expand Down Expand Up @@ -575,20 +597,6 @@ describe( 'Navigation', () => {
const NAV_ENTITY_SELECTOR =
'//div[@class="entities-saved-states__panel"]//label//strong[contains(text(), "Navigation")]';

async function populateNavWithOneItem() {
// Add a Link block first.
const appender = await page.waitForSelector(
'.wp-block-navigation .block-list-appender'
);
await appender.click();
// Add a link to the Link block.
await updateActiveNavigationLink( {
url: 'https://wordpress.org',
label: 'WP',
type: 'url',
} );
}

async function resetNavBlockToInitialState() {
const selectMenuDropdown = await page.waitForSelector(
'[aria-label="Select Menu"]'
Expand Down Expand Up @@ -731,6 +739,112 @@ describe( 'Navigation', () => {
expect( tagCount ).toBe( 1 );
} );

// eslint-disable-next-line jest/no-focused-tests
describe.only( 'Submenus', () => {
it( 'shows button which converts submenu to link when submenu is not-populated (empty)', async () => {
const navSubmenuSelector = `[aria-label="Editor content"][role="region"] [aria-label="Block: Submenu"]`;

await createNewPost();
await insertBlock( 'Navigation' );

const startEmptyButton = await page.waitForXPath(
START_EMPTY_XPATH
);

await startEmptyButton.click();

await populateNavWithOneItem();

await clickBlockToolbarButton( 'Add submenu' );

await waitForBlock( 'Submenu' );

// Revert the Submenu back to a Navigation Link block.
await clickBlockToolbarButton( 'Convert to Link' );

// Check the Submenu block is no longer present.
const convertToLinkButton = await page.$( navSubmenuSelector );

expect( convertToLinkButton ).toBeFalsy();
} );

it( 'does not show button to convert submenu to link when submenu is populated', async () => {
await createNewPost();
await insertBlock( 'Navigation' );

const startEmptyButton = await page.waitForXPath(
START_EMPTY_XPATH
);

await startEmptyButton.click();

await populateNavWithOneItem();

await clickBlockToolbarButton( 'Add submenu' );

await waitForBlock( 'Submenu' );

// Add a Link block first.
const appender = await page.waitForSelector(
'[aria-label="Block: Submenu"] [aria-label="Add block"]'
);

await appender.click();

await updateActiveNavigationLink( {
url: 'https://make.wordpress.org/core/',
label: 'Submenu item #1',
type: 'url',
} );

await clickBlockToolbarButton( 'Select Submenu' );

const convertToLinkButton = await page.$(
'[aria-label="Block tools"] [aria-label="Convert to Link"]'
);

expect( convertToLinkButton ).toBeFalsy();
} );

it( 'shows button to convert submenu to link when submenu is populated with a single incomplete link item', async () => {
// For context on why this test is required please see:
// https://github.com/WordPress/gutenberg/pull/38203#issuecomment-1027672948.

await createNewPost();
await insertBlock( 'Navigation' );

const startEmptyButton = await page.waitForXPath(
START_EMPTY_XPATH
);

await startEmptyButton.click();

await populateNavWithOneItem();

await clickBlockToolbarButton( 'Add submenu' );

await waitForBlock( 'Submenu' );

// Add a Link block first.
const appender = await page.waitForSelector(
'[aria-label="Block: Submenu"] [aria-label="Add block"]'
);

await appender.click();

// Here we intentionally do not populate the inserted Navigation Link block.
// Rather we immediaely click away leaving the link in a state where it has
// no URL of label and can be considered unpopulated.
await clickBlockToolbarButton( 'Select Submenu' );

const convertToLinkButton = await page.$(
'[aria-label="Block tools"] [aria-label="Convert to Link"]'
);

expect( convertToLinkButton ).toBeTruthy();
} );
} );

describe( 'Permission based restrictions', () => {
afterEach( async () => {
await switchUserToAdmin();
Expand Down
1 change: 1 addition & 0 deletions packages/icons/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ export { default as quote } from './library/quote';
export { default as receipt } from './library/receipt';
export { default as redo } from './library/redo';
export { default as removeBug } from './library/remove-bug';
export { default as removeSubmenu } from './library/remove-submenu';
export { default as replace } from './library/replace';
export { default as reset } from './library/reset';
export { default as resizeCornerNE } from './library/resize-corner-n-e';
Expand Down
16 changes: 16 additions & 0 deletions packages/icons/src/library/remove-submenu.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { SVG, Path } from '@wordpress/primitives';

const removeSubmenu = (
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24">
<Path
fill-rule="evenodd"
clip-rule="evenodd"
d="m13.955 20.748 8-17.5-.91-.416L19.597 6H13.5v1.5h5.411l-1.6 3.5H13.5v1.5h3.126l-1.6 3.5H13.5l.028 1.5h.812l-1.295 2.832.91.416ZM17.675 16l-.686 1.5h4.539L21.5 16h-3.825Zm2.286-5-.686 1.5H21.5V11h-1.54ZM2 12c0 3.58 2.42 5.5 6 5.5h.5V19l3-2.5-3-2.5v2H8c-2.48 0-4.5-1.52-4.5-4S5.52 7.5 8 7.5h3.5V6H8c-3.58 0-6 2.42-6 6Z"
/>
</SVG>
);

export default removeSubmenu;