From 2d61c0b32a279e4382988ee7dacee66a7ca5dad0 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 24 Feb 2023 06:44:18 +0000 Subject: [PATCH] Revert Link control UX changes for WP 6.2 (#48359) * Revert "Move Link Control action buttons into lower area (#47309)" This reverts commit e6059372fee79ca56e5d917ef3c20525131e619a. * Revert "Fix Link Control action button visuals (#47306)" This reverts commit 2499646e1dff9a82520749eb17d96cf85c8a0a1a. * Revert "Add clear Apply and Cancel buttons to Link Control (#46933)" This reverts commit 875628f63a84abc5d60efc727994b75547ab6a5e. # Conflicts: # packages/block-editor/src/components/link-control/index.js --- .../src/components/link-control/index.js | 81 +++------- .../src/components/link-control/style.scss | 30 +++- .../src/components/link-control/test/index.js | 139 +----------------- .../media-replace-flow/test/index.js | 2 +- .../block-library/src/navigation-link/edit.js | 15 +- .../various/__snapshots__/links.test.js.snap | 2 +- .../specs/editor/various/links.test.js | 8 +- test/e2e/specs/editor/blocks/image.spec.js | 2 +- 8 files changed, 63 insertions(+), 216 deletions(-) diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 73e2f6bb8e0e1..2d0f0078a9e76 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -7,6 +7,7 @@ import classnames from 'classnames'; * WordPress dependencies */ import { Button, Spinner, Notice, TextControl } from '@wordpress/components'; +import { keyboardReturn } from '@wordpress/icons'; import { __ } from '@wordpress/i18n'; import { useRef, useState, useEffect } from '@wordpress/element'; import { focus } from '@wordpress/dom'; @@ -112,7 +113,6 @@ function LinkControl( { settings = DEFAULT_LINK_SETTINGS, onChange = noop, onRemove, - onCancel, noDirectEntry = false, showSuggestions = true, showInitialSuggestions, @@ -190,8 +190,6 @@ function LinkControl( { isEndingEditWithFocus.current = false; }, [ isEditingLink, isCreatingPage ] ); - const hasLinkValue = value?.url?.trim()?.length > 0; - /** * Cancels editing state and marks that focus may need to be restored after * the next render, if focus was within the wrapper when editing finished. @@ -237,29 +235,6 @@ function LinkControl( { } }; - const resetInternalValues = () => { - setInternalUrlInputValue( value?.url ); - setInternalTextInputValue( value?.title ); - }; - - const handleCancel = ( event ) => { - event.preventDefault(); - event.stopPropagation(); - - // Ensure that any unsubmitted input changes are reset. - resetInternalValues(); - - if ( hasLinkValue ) { - // If there is a link then exist editing mode and show preview. - stopEditing(); - } else { - // If there is no link value, then remove the link entirely. - onRemove?.(); - } - - onCancel?.(); - }; - const currentUrlInputValue = propInputValue || internalUrlInputValue; const currentInputIsEmpty = ! currentUrlInputValue?.trim()?.length; @@ -272,9 +247,8 @@ function LinkControl( { // Only show text control once a URL value has been committed // and it isn't just empty whitespace. // See https://github.com/WordPress/gutenberg/pull/33849/#issuecomment-932194927. - const showTextControl = hasLinkValue && hasTextControl; + const showTextControl = value?.url?.trim()?.length > 0 && hasTextControl; - const isEditing = ( isEditingLink || ! value ) && ! isCreatingPage; return (
) } - { isEditing && ( + { ( isEditingLink || ! value ) && ! isCreatingPage && ( <>
+ > +
+
+
{ errorMessage && ( ) } -
- { showSettingsDrawer && ( -
- -
- ) } - - { isEditing && ( -
- - -
- ) } -
- + { showSettingsDrawer && ( +
+ +
+ ) } { renderControlBottom && renderControlBottom() }
); diff --git a/packages/block-editor/src/components/link-control/style.scss b/packages/block-editor/src/components/link-control/style.scss index cadb0ce6340e2..a34d050353f94 100644 --- a/packages/block-editor/src/components/link-control/style.scss +++ b/packages/block-editor/src/components/link-control/style.scss @@ -64,6 +64,7 @@ $preview-image-height: 140px; width: calc(100% - #{$grid-unit-20 * 2}); display: block; padding: 11px $grid-unit-20; + padding-right: ( $button-size * $block-editor-link-control-number-of-actions ); // width of reset and submit buttons margin: 0; position: relative; border: 1px solid $gray-300; @@ -76,10 +77,20 @@ $preview-image-height: 140px; } .block-editor-link-control__search-actions { - display: flex; - flex-direction: row-reverse; // put "Cancel" on the left but retain DOM order. - justify-content: flex-start; - gap: $grid-unit-10; + position: absolute; + /* + * Actions must be positioned on top of URLInput, since the input will grow + * when suggestions are rendered. + * + * Compensate for: + * - Border (1px) + * - Vertically, for the difference in height between the input (40px) and + * the icon buttons. + * - Horizontally, pad to the minimum of: default input padding, or the + * equivalent of the vertical padding. + */ + top: 1px + ( ( 40px - $button-size ) * 0.5 ); + right: $grid-unit-20 + 1px + min($grid-unit-10, ( 40px - $button-size ) * 0.5); } .components-button .block-editor-link-control__search-submit .has-icon { @@ -426,10 +437,9 @@ $preview-image-height: 140px; padding: 10px; } -.block-editor-link-control__drawer { +.block-editor-link-control__tools { display: flex; align-items: center; - justify-content: space-between; border-top: $border-width solid $gray-300; margin: 0; padding: $grid-unit-20; @@ -469,8 +479,14 @@ $preview-image-height: 140px; position: absolute; left: auto; bottom: auto; + /* + * Position spinner to the left of the actions. + * + * Compensate for: + * - Input padding right ($button-size) + */ top: calc(50% - #{$spinner-size} / 2); - right: $grid-unit-20; + right: $button-size; } } diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js index bf48d93305fca..86eae7289f4f6 100644 --- a/packages/block-editor/src/components/link-control/test/index.js +++ b/packages/block-editor/src/components/link-control/test/index.js @@ -537,7 +537,7 @@ describe( 'Manual link entry', () => { } ); let submitButton = screen.getByRole( 'button', { - name: 'Apply', + name: 'Submit', } ); expect( submitButton ).toBeDisabled(); @@ -555,7 +555,7 @@ describe( 'Manual link entry', () => { await user.keyboard( '[Enter]' ); submitButton = screen.getByRole( 'button', { - name: 'Apply', + name: 'Submit', } ); // Verify the UI hasn't allowed submission. @@ -578,7 +578,7 @@ describe( 'Manual link entry', () => { } ); let submitButton = screen.queryByRole( 'button', { - name: 'Apply', + name: 'Submit', } ); expect( submitButton ).toBeDisabled(); @@ -597,7 +597,7 @@ describe( 'Manual link entry', () => { await user.click( submitButton ); submitButton = screen.queryByRole( 'button', { - name: 'Apply', + name: 'Submit', } ); // Verify the UI hasn't allowed submission. @@ -608,135 +608,6 @@ describe( 'Manual link entry', () => { ); } ); - describe( 'Handling cancellation', () => { - it( 'should allow cancellation of the link creation process and reset any entered values', async () => { - const user = userEvent.setup(); - const mockOnRemove = jest.fn(); - const mockOnCancel = jest.fn(); - - render( ); - - // Search Input UI. - const searchInput = screen.getByRole( 'combobox', { - name: 'URL', - } ); - - const cancelButton = screen.queryByRole( 'button', { - name: 'Cancel', - } ); - - expect( cancelButton ).toBeEnabled(); - expect( cancelButton ).toBeVisible(); - - // Simulate adding a link for a term. - await user.type( searchInput, 'https://www.wordpress.org' ); - - // Attempt to submit the empty search value in the input. - await user.click( cancelButton ); - - // Verify the consumer can handle the cancellation. - expect( mockOnRemove ).toHaveBeenCalled(); - - // Ensure optional callback is not called. - expect( mockOnCancel ).not.toHaveBeenCalled(); - - expect( searchInput ).toHaveValue( '' ); - } ); - - it( 'should allow cancellation of the link editing process and reset any entered values', async () => { - const user = userEvent.setup(); - const initialLink = fauxEntitySuggestions[ 0 ]; - - const LinkControlConsumer = () => { - const [ link, setLink ] = useState( initialLink ); - - return ( - { - setLink( suggestion ); - } } - hasTextControl - /> - ); - }; - - render( ); - - let linkPreview = screen.getByLabelText( 'Currently selected' ); - - expect( linkPreview ).toBeInTheDocument(); - - // Click the "Edit" button to trigger into the editing mode. - let editButton = screen.queryByRole( 'button', { - name: 'Edit', - } ); - - await user.click( editButton ); - - let searchInput = screen.getByRole( 'combobox', { - name: 'URL', - } ); - - let textInput = screen.getByRole( 'textbox', { - name: 'Text', - } ); - - // Make a change to the search input. - await user.type( searchInput, 'This URL value was changed!' ); - - // Make a change to the text input. - await user.type( textInput, 'This text value was changed!' ); - - const cancelButton = screen.queryByRole( 'button', { - name: 'Cancel', - } ); - - // Cancel the editing process. - await user.click( cancelButton ); - - linkPreview = screen.getByLabelText( 'Currently selected' ); - - expect( linkPreview ).toBeInTheDocument(); - - // Re-query the edit button as it's been replaced. - editButton = screen.queryByRole( 'button', { - name: 'Edit', - } ); - - await user.click( editButton ); - - // Re-query the inputs as they have been replaced. - searchInput = screen.getByRole( 'combobox', { - name: 'URL', - } ); - - textInput = screen.getByRole( 'textbox', { - name: 'Text', - } ); - - // Expect to see the original link values and **not** the changed values. - expect( searchInput ).toHaveValue( initialLink.url ); - expect( textInput ).toHaveValue( initialLink.text ); - } ); - - it( 'should call onCancel callback when cancelling if provided', async () => { - const user = userEvent.setup(); - const mockOnCancel = jest.fn(); - - render( ); - - const cancelButton = screen.queryByRole( 'button', { - name: 'Cancel', - } ); - - await user.click( cancelButton ); - - // Verify the consumer can handle the cancellation. - expect( mockOnCancel ).toHaveBeenCalled(); - } ); - } ); - describe( 'Alternative link protocols and formats', () => { it.each( [ [ 'mailto:example123456@wordpress.org', 'mailto' ], @@ -1988,7 +1859,7 @@ describe( 'Controlling link title text', () => { expect( textInput ).toHaveValue( textValue ); const submitButton = screen.queryByRole( 'button', { - name: 'Apply', + name: 'Submit', } ); await user.click( submitButton ); diff --git a/packages/block-editor/src/components/media-replace-flow/test/index.js b/packages/block-editor/src/components/media-replace-flow/test/index.js index 9d1aef6df7620..2d3a9f64fc198 100644 --- a/packages/block-editor/src/components/media-replace-flow/test/index.js +++ b/packages/block-editor/src/components/media-replace-flow/test/index.js @@ -137,7 +137,7 @@ describe( 'General media replace flow', () => { await user.click( screen.getByRole( 'button', { - name: 'Apply', + name: 'Submit', } ) ); diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index fda1268715c32..3ac89bb881965 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -315,17 +315,12 @@ export default function NavigationLinkEdit( { */ function removeLink() { // Reset all attributes that comprise the link. - // It is critical that all attributes are reset - // to their default values otherwise this may - // in advertently trigger side effects because - // the values will have "changed". setAttributes( { - url: undefined, - label: undefined, - id: undefined, - kind: undefined, - type: undefined, - opensInNewTab: false, + url: '', + label: '', + id: '', + kind: '', + type: '', } ); // Close the link editing UI. diff --git a/packages/e2e-tests/specs/editor/various/__snapshots__/links.test.js.snap b/packages/e2e-tests/specs/editor/various/__snapshots__/links.test.js.snap index 6ba1f2c014885..330dfbbe142b0 100644 --- a/packages/e2e-tests/specs/editor/various/__snapshots__/links.test.js.snap +++ b/packages/e2e-tests/specs/editor/various/__snapshots__/links.test.js.snap @@ -68,6 +68,6 @@ exports[`Links should contain a label when it should open in a new tab 1`] = ` exports[`Links should contain a label when it should open in a new tab 2`] = ` " -

This is WordPress

+

This is WordPress

" `; diff --git a/packages/e2e-tests/specs/editor/various/links.test.js b/packages/e2e-tests/specs/editor/various/links.test.js index 9ffbd6aa041ca..b9f5a2a990cb9 100644 --- a/packages/e2e-tests/specs/editor/various/links.test.js +++ b/packages/e2e-tests/specs/editor/various/links.test.js @@ -121,6 +121,7 @@ describe( 'Links', () => { // Navigate to and toggle the "Open in new tab" checkbox. await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Space' ); // Toggle should still have focus and be checked. @@ -133,7 +134,7 @@ describe( 'Links', () => { expect( await getEditedPostContent() ).toMatchSnapshot(); // Tab back to the Submit and apply the link. - await page.keyboard.press( 'Tab' ); + await pressKeyWithModifier( 'shift', 'Tab' ); await page.keyboard.press( 'Enter' ); // The link should have been inserted. @@ -526,6 +527,7 @@ describe( 'Links', () => { // Navigate to and toggle the "Open in new tab" checkbox. await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Space' ); // Confirm that focus was not prematurely returned to the paragraph on @@ -534,8 +536,7 @@ describe( 'Links', () => { // Close dialog. Expect that "Open in new tab" would have been applied // immediately. - - await pressKeyWithModifier( 'shift', 'Tab' ); + await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Enter' ); // Wait for Gutenberg to finish the job. @@ -765,7 +766,6 @@ describe( 'Links', () => { await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Tab' ); - await page.keyboard.press( 'Tab' ); // Make a selection within the RichText. await pressKeyWithModifier( 'shift', 'ArrowRight' ); diff --git a/test/e2e/specs/editor/blocks/image.spec.js b/test/e2e/specs/editor/blocks/image.spec.js index fc435544b21bb..725062cc8e31d 100644 --- a/test/e2e/specs/editor/blocks/image.spec.js +++ b/test/e2e/specs/editor/blocks/image.spec.js @@ -498,7 +498,7 @@ test.describe( 'Image', () => { await page.click( 'role=button[name="Edit"i]' ); // Replace the url. await page.fill( 'role=combobox[name="URL"i]', imageUrl ); - await page.click( 'role=button[name="Apply"i]' ); + await page.click( 'role=button[name="Submit"i]' ); const regex = new RegExp( `