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

Add clear Apply and Cancel buttons to Link Control #46933

Merged
merged 16 commits into from
Jan 19, 2023

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 6, 2023

What?

Adds Apply and Cancel buttons to the <LinkControl> component.

Affords the ability to explicitly cancel the creation or editing of the link.

Closes #46593

Why?

TLDR

  • it needs to be clearer how to commit and cancel creation/editing of links. Currently it's unclear and unintuitive.
  • Addition of clear unambiguous buttons greatly improves the experience.
  • Specific "Close" button is required for a11y reasons (click away or cancel on blur is not sufficient).

There are a number of small UX issues with not having clear buttons to apply or cancel a link. Specifically there have been quite a few issues reporting around inconsistencies and bugs when submitting changes and UX confusion about how/when changes should be committed.

Examples

This PR is a first step in addressing these issues. See Next Steps for followup ideas.

Editing an existing link

When you have an existing link and you make changes to the text or URL it's not immediately obvious how to submit those changes. In fact you have to click on the very small icon button which is inside the URL input. Questions that might arrise:

  • Where on earth(!) is the submit button?
  • Does clicking submit on the URL input submit my changes to the Text value of the link?

By adding a clear Apply button it's

  • clear where the button is that "commits" the changes
  • clear that one button will commit all the changes.

Cancelling edits to an existing link

When you have an existing link and you make changes to the text or URL and then decide you don't want to commit those changes it's very unclear how to exit the editing process.

In fact you have to click outside of the component to close it. But that's not at all intuitive.

With a dedicated Cancel button it is immediately clear how to exit the editing process and all ambiguity is removed.

Cancelling the creation of a new link

When you are creating a new link and then decide that in fact you do not wish to do this it's not immediately clear how to exit the link creation process.

In fact you have to click outside of the component to close it. But that's not at all intuitive and can even lead to focus loss from the place where you were originally creating the link which is extremely frustrating.

With a dedicated Cancel button it is immediately clear how to exit the editing process and all ambiguity is removed. Moreover your focus is returned back to where you were originally editing which avoids UX issues related to that.

How?

Adds x2 new buttons

  • Apply - this replaces the icon-base submit button but displays all of the same behaviours and allows for submitting any current changes to the link value.
  • Cancel - this is new and provides a clear means for the user to cancel editing or creation of links.

Care has been taken to ensure that when cancelling an action any _un_committed edits to a link value are discarded.

Next Steps

If this PR lands then the next step would be to make changes to the link settings (e.g. Open in new tab) to be explicitly committed by the user.

Currently these controls require lots of custom code to handle when they should/shouldn't be committed and it's never resulted in a satisfactory UX.

We can greatly improve the UX simply by having a rule than any changes to the link value must be explicitly committed bu the user.

Testing Instructions

Manual testing

  • New Post
  • Add some text
  • Create a new link
  • See new UI with Apply and Cancel buttons
  • Click Cancel - see link creation is cancelled and focus returned to previous location.
  • Create a new link and submit a value.
  • Click the edit button on the link you created.
  • Make some text changes and URL changes then click Cancel.
  • See the changes you made have been discarded.

Repeat the above with other locations where LinkControl is used such as

  • Nav block
  • Button block
  • Image block

...etc

Automated testing

Review and then run the tests:

npm run test:unit packages/block-editor/src/components/link-control/test/index.js

Testing Instructions for Keyboard

Repeat same tests as above but ensure that you can complete the tasks using the keyboard only.

Specifically check that the tab order of the Link Control allows for Apply as the first tab stop (and thus the primary option) but also allows access to Cancel as the secondary stop.

Screenshots or screencast

Screenshot 2023-01-06 at 13 35 07

Before

Screen.Capture.on.2023-01-06.at.10-51-27.mp4

After

Screen.Capture.on.2023-01-06.at.11-08-49.mp4

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Size Change: +84 B (0%)

Total Size: 1.33 MB

Filename Size Change
build/block-editor/index.min.js 185 kB +75 B (0%)
build/block-editor/style.css 14.2 kB +2 B (0%)
build/block-library/index.min.js 199 kB +7 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.78 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 483 B
build/block-directory/index.min.js 7.16 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 3.65 kB
build/block-editor/content.css 3.65 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-editor/style-rtl.css 14.2 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 138 B
build/block-library/blocks/audio/theme.css 138 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 84 B
build/block-library/blocks/avatar/style.css 84 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 485 B
build/block-library/blocks/button/editor.css 485 B
build/block-library/blocks/button/style-rtl.css 532 B
build/block-library/blocks/button/style.css 532 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 612 B
build/block-library/blocks/cover/editor.css 613 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.56 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 138 B
build/block-library/blocks/embed/theme.css 138 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 353 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 984 B
build/block-library/blocks/gallery/editor.css 988 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 829 B
build/block-library/blocks/image/editor.css 828 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 298 B
build/block-library/blocks/latest-comments/style.css 298 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 716 B
build/block-library/blocks/navigation-link/editor.css 715 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.13 kB
build/block-library/blocks/navigation/editor.css 2.14 kB
build/block-library/blocks/navigation/style-rtl.css 2.22 kB
build/block-library/blocks/navigation/style.css 2.2 kB
build/block-library/blocks/navigation/view-modal.min.js 2.81 kB
build/block-library/blocks/navigation/view.min.js 447 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 376 B
build/block-library/blocks/page-list/editor.css 376 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 501 B
build/block-library/blocks/post-comments-form/style.css 501 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 586 B
build/block-library/blocks/post-featured-image/editor.css 584 B
build/block-library/blocks/post-featured-image/style-rtl.css 318 B
build/block-library/blocks/post-featured-image/style.css 318 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 440 B
build/block-library/blocks/query/editor.css 440 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 490 B
build/block-library/blocks/site-logo/editor.css 490 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 433 B
build/block-library/blocks/table/editor.css 433 B
build/block-library/blocks/table/style-rtl.css 651 B
build/block-library/blocks/table/style.css 650 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 404 B
build/block-library/blocks/template-part/editor.css 404 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 691 B
build/block-library/blocks/video/editor.css 694 B
build/block-library/blocks/video/style-rtl.css 179 B
build/block-library/blocks/video/style.css 179 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 162 B
build/block-library/classic.css 162 B
build/block-library/common-rtl.css 1.05 kB
build/block-library/common.css 1.05 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 11.7 kB
build/block-library/editor.css 11.7 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.4 kB
build/block-library/style.css 12.4 kB
build/block-library/theme-rtl.css 698 B
build/block-library/theme.css 703 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 50.4 kB
build/components/index.min.js 203 kB
build/components/style-rtl.css 11.6 kB
build/components/style.css 11.7 kB
build/compose/index.min.js 12.3 kB
build/core-data/index.min.js 15.9 kB
build/customize-widgets/index.min.js 11.7 kB
build/customize-widgets/style-rtl.css 1.41 kB
build/customize-widgets/style.css 1.41 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 7.95 kB
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.71 kB
build/edit-navigation/index.min.js 16.2 kB
build/edit-navigation/style-rtl.css 4.14 kB
build/edit-navigation/style.css 4.15 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-post/index.min.js 34.4 kB
build/edit-post/style-rtl.css 7.46 kB
build/edit-post/style.css 7.45 kB
build/edit-site/index.min.js 66.5 kB
build/edit-site/style-rtl.css 9.38 kB
build/edit-site/style.css 9.38 kB
build/edit-widgets/index.min.js 16.8 kB
build/edit-widgets/style-rtl.css 4.49 kB
build/edit-widgets/style.css 4.49 kB
build/editor/index.min.js 44.1 kB
build/editor/style-rtl.css 3.68 kB
build/editor/style.css 3.67 kB
build/element/index.min.js 4.93 kB
build/escape-html/index.min.js 548 B
build/experiments/index.min.js 862 B
build/format-library/index.min.js 7.2 kB
build/format-library/style-rtl.css 598 B
build/format-library/style.css 597 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.79 kB
build/keycodes/index.min.js 1.88 kB
build/list-reusable-blocks/index.min.js 2.14 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.99 kB
build/notices/index.min.js 977 B
build/plugins/index.min.js 1.95 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.35 kB
build/primitives/index.min.js 960 B
build/priority-queue/index.min.js 1.59 kB
build/react-i18n/index.min.js 702 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.75 kB
build/reusable-blocks/index.min.js 2.27 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/rich-text/index.min.js 10.8 kB
build/server-side-render/index.min.js 2.09 kB
build/shortcode/index.min.js 1.52 kB
build/style-engine/index.min.js 1.53 kB
build/token-list/index.min.js 650 B
build/url/index.min.js 3.69 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 1.09 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.31 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@getdave getdave self-assigned this Jan 6, 2023
@getdave getdave added the [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) label Jan 6, 2023
@getdave getdave marked this pull request as ready for review January 6, 2023 11:10
@getdave getdave requested a review from ellatrix as a code owner January 6, 2023 11:10
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Flaky tests detected in ee46cf2.
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/3955139415
📝 Reported issues:

@getdave
Copy link
Contributor Author

getdave commented Jan 6, 2023

If anyone notices the misaligned labels on the control I fixed that in #46936

@getdave getdave added Needs Design Feedback Needs general design feedback. [Type] Feature New feature to highlight in changelogs. labels Jan 6, 2023
@draganescu
Copy link
Contributor

Just a quick take: I like the substrate of this PR and I agree with the problem it tries to solve, illustrated well in the 1st video. However I think the two buttons are just too much. The improvements seem to be more likely in:

  • reflecting visible updates when they happen
  • adding a smaller, secondary way to exit with no change, maybe some [x]

@jameskoster jameskoster requested a review from a team January 6, 2023 11:47
@getdave
Copy link
Contributor Author

getdave commented Jan 6, 2023

FYI I've had an Issue up for while about this but didn't make much effort to get it traction.

@getdave
Copy link
Contributor Author

getdave commented Jan 6, 2023

Just a quick take: I like the substrate of this PR and I agree with the problem it tries to solve, illustrated well in the 1st video. However I think the two buttons are just too much. The improvements seem to be more likely in:

  • reflecting visible updates when they happen
  • adding a smaller, secondary way to exit with no change, maybe some [x]

I'm no designer. However whatever route design suggests it needs to cover the scenarios outlined above. Currently everything relies on "discovering" how to do things.

IMHO it should be clear without having to learn or undue cognitive load, even if that's at the expense of aesthetics.

A critical point to note is that (in the future) I'm suggesting making all changes to links require the user to explicitly submit them. The key example is that I should be able to toggle the Open in new tab switch and not have that applied until I click Apply.

If you look at the code for how all the various implementations of "link creation" consume <LinkControl> you'll note that each handles the toggle scenario slightly differently. These minor inconsistencies cause bug reports and everyone expects it to behave differently depending on their own context.

Standardising on "user must explicitly commit all changes to links" makes it a lot clearer.

However I think the two buttons are just too much.

Just noting that the x2 buttons only appear when editing. Here are some examples in the wild that use clear buttons:

Screen Shot 2022-12-15 at 21 23 46 (1)

Screen Shot 2023-01-06 at 13 30 07

adding a smaller, secondary way to exit with no change, maybe some [x]

IMHO a x is not equivalent to "Cancel". It means dismiss but it's still ambiguous about what will happen to any changes that have been made.

@draganescu
Copy link
Contributor

A critical point to note is that (in the future) I'm suggesting making all changes to links require the user to explicitly submit them. The key example is that I should be able to toggle the Open in new tab switch and not have that applied until I click Apply.

I think this sort of thing is not expected anymore for quite some time, because UX has more and more discarded apply in favor of easy undo. Apply should be for destructive or hard to undo things, like a sequence of updates where you can lose track of what you did. A toggle is on when you toggled it on in just about all places from system settings of OSes to browser settings and so on.

@jameskoster
Copy link
Contributor

I also appreciate the sentiment of this change and agree the link editing experience can feel a bit ambiguous. But I'm also not sure that the solution is as simple as universally adding these two buttons.

One thing that works well at the moment is that clicking a search result is equivalent to an "Apply". It's a nice confirmatory action and it would be a shame to introduce any additional clicks there.

Taking a step back, the problem seems to be that non-confirmatory closes of the link control do not feel predictable in terms of end result. IE if you click outside the popover there's no indication as to whether your changes were saved or not. This is particularly bad when editing a link because there's no visual feedback on the canvas.

All that considered, an alternative idea to try:

Screenshot 2023-01-06 at 16 17 53

Clicking a result (or pressing enter) is the only way to confirm a selection, doing so closes the link control. The Cancel button will cancel the addition of a new link, or discard any edits to an existing one. Consequently there are only two actions that can close the link control, and both feel predictable. Having typed that out I wonder if a popover is the correct pattern here, and whether a modal would work better.

@richtabor
Copy link
Member

I also appreciate the sentiment of this change and agree the link editing experience can feel a bit ambiguous. But I'm also not sure that the solution is as simple as universally adding these two buttons.

Agree.

One thing that works well at the moment is that clicking a search result is equivalent to an "Apply". It's a nice confirmatory action and it would be a shame to introduce any additional clicks there.

I would think "Apply" and clicking on a result would both complete the same action.

With a dedicated Cancel button it is immediately clear how to exit the editing process and all ambiguity is removed.

It does add clarity, though clicking elsewhere to cancel/close a popover is a standard UX practice. If it's unfinished/unconfirmed, then it resets.

What about if we add just the Apply button, and if one does not click "Apply", press "Enter" (which is how it works today), or click on one of the search results, then neither an initial link - or link edit - should save?

@richtabor
Copy link
Member

richtabor commented Jan 6, 2023

Clicking a result (or pressing enter) is the only way to confirm a selection, doing so closes the link control. The Cancel button will cancel the addition of a new link, or discard any edits to an existing one.

My suggestion above is basically the opposite :)

No "Cancel", but better confirmation via "Apply" (still keep the results, and enter flow).

@richtabor

This comment was marked as duplicate.

@jameskoster
Copy link
Contributor

No "Cancel", but better confirmation via "Apply" (still keep the results, and enter flow).

What would the flow to add a link look like? It seems to be:

  1. Click add link tool
  2. Search / type url
  3. Click result

In this case wouldn't the "Apply" button be redundant?

For this to work I suppose the "Apply" button might only appear when editing an existing link. But still, if clicking a result is akin to clicking "Apply" then it still seems redundant 🤔

though clicking elsewhere to cancel/close a popover is a standard UX practice

It is, but I think the problem here is that when you do so it's unclear whether the result is a cancel or save action? You have to open the link control again to check. It's not so bad when adding a link because you get visual feedback to confirm the link was added (link styles appear). But if you're editing a link there's no such feedback.


Another idea: Adding a link works as it does now. Editing a link takes place in a modal. Very crude mockup:

Screenshot 2023-01-06 at 18 09 27

@getdave
Copy link
Contributor Author

getdave commented Jan 6, 2023

Secondary, but we should also fix this:

CleanShot 2023-01-06 at 11 57 23@2x

You must have missed #46933 (comment) ?

@getdave
Copy link
Contributor Author

getdave commented Jan 6, 2023

This one is quite complex. There are a number of different scenarios to consider other than just creating a new link.

I will say that this PR does retain the existing functionality which is when you select an option it automatically selects that link (no need for Apply here).

However when editing a link or creating a link that's a URL (e.g. you type in www.wordpress.org) you need a means to submit. That's why we already have a Submit button on trunk.

Therefore I'd say the main changes to consider in this PR are really

  • making the Apply button much more obvious (as I said it already exists).
  • introducing an explicit Cancel action.

Therefore the questions we need to consider are:

  • is an explicit Cancel button better than a potentially ambiguous requirement to click outside to close?
  • is a clear Apply button better than a small icon button for "submit".

I appreciate this is a difficult problem so I appreciate everyone chipping in here to aid in improving the experience. It's badly needed for the reasons I've stated in the PR desc.

@scruffian
Copy link
Contributor

I agree that we need different things when editing vs creating a new link. For example if I'm creating a new link as below, it's hard to imagine a situation when we'd ever need those buttons:
Screenshot 2023-01-06 at 21 21 57

However when editing a link the need is quite clear:
Screenshot 2023-01-06 at 21 22 07

I think this is also wrapped up in some of the changed being discussed at https://github.com/WordPress/gutenberg/pull/46891/files#top, where we also want to consider changing the experience for editing links, so a more thorough design exploration would be beneficial.

@richtabor
Copy link
Member

What would the flow to add a link look like? It seems to be:

Click add link tool
Search / type url
Click result
In this case wouldn't the "Apply" button be redundant?

Yes. I think it's fine. You're still confirming the action, either by clicking a result - or adding a custom url and applying it.

It is, but I think the problem here is that when you do so it's unclear whether the result is a cancel or save action?

I'm thinking it should only be saved if you've taken action in the popover. I.e. You've applied a custom link, or selected a page via the results.

@alexstine
Copy link
Contributor

This is one thing that annoys me about Gutenberg and other applications that work in similar ways. I do not want to guess if my action was committed or canceled, having a clearly defined set of buttons for this eliminates all mystery. I was never in favor of putting visuals above good UX. Where do we draw the line... I think this is a good path to go down and hopefully these common sense controls can be brought to other components. I'll A11Y review this when I have a moment.

Thanks.

@getdave getdave force-pushed the add/clear-action-buttons-to-link-control branch from e7b4e82 to c900773 Compare January 19, 2023 02:47
@@ -598,7 +579,7 @@ export default function NavigationLinkEdit( {
onClose={ () => setIsLinkOpen( false ) }
anchor={ popoverAnchor }
hasCreateSuggestion={ userCanCreate }
onRemove={ removeLink }
onRemove={ () => removeBlocks( clientId ) }
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 don't think we should do this in this PR. It has knock on consequences. For example, it will not allow folks to add empty link items which I believe was functionality that was specifically requsted by users in the past.

The use case is that you might want to build out the structure of a Nav without necessarily adding links to each item.

With this change any nav link block that doesn't add a link using the initial Link UI popup will be removed.

Happy to consider this but I think it should be in a separate PR to avoid over burdening this PR 🙏

@scruffian scruffian force-pushed the add/clear-action-buttons-to-link-control branch from d6acce6 to ee46cf2 Compare January 19, 2023 03:41
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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`] = `
"<!-- wp:paragraph -->
<p>This is <a href=\\"http://wordpress.org\\">WordPress</a></p>
<p>This is <a href=\\"http://wordpress.org\\" target=\\"_blank\\" rel=\\"noreferrer noopener\\">WordPress</a></p>
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 believe this snapshot has been invalid in trunk for a while. This is because the test sets open in new tab to it's active state which should result in the attributes being added to the HTML.

@scruffian scruffian merged commit 875628f into trunk Jan 19, 2023
@scruffian scruffian deleted the add/clear-action-buttons-to-link-control branch January 19, 2023 06:32
@github-actions github-actions bot added this to the Gutenberg 15.1 milestone Jan 19, 2023
@@ -235,6 +243,29 @@ function LinkControl( {
}
};

const resetInternalValues = () => {
Copy link
Contributor

@draganescu draganescu Jan 25, 2023

Choose a reason for hiding this comment

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

Nit: do we need this function? Seems to be called once.

@juanmaguitar juanmaguitar added the [Package] Block editor /packages/block-editor label Feb 1, 2023
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Feb 7, 2023
ntsekouras pushed a commit that referenced this pull request Feb 23, 2023
This reverts commit 875628f.

# Conflicts:
#	packages/block-editor/src/components/link-control/index.js
ntsekouras pushed a commit that referenced this pull request Feb 24, 2023
* Revert "Move Link Control action buttons into lower area (#47309)"

This reverts commit e605937.

* Revert "Fix Link Control action button visuals (#47306)"

This reverts commit 2499646.

* Revert "Add clear Apply and Cancel buttons to Link Control (#46933)"

This reverts commit 875628f.

# Conflicts:
#	packages/block-editor/src/components/link-control/index.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. Needs User Documentation Needs new user documentation [Package] Block editor /packages/block-editor [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add clear Submit and Cancel buttons to Link UI