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 settings "drawer" to Link Control #47328

Merged
merged 23 commits into from
Feb 7, 2023

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 21, 2023

What?

Implements a accordion-like panel and toggle for link settings.

All settings + the link text control are now hidden in the panel by default which must be expanded to reveal the settings.

Part of #47310

Why?

As shown in the revised design in #47310 this is important in order to accommodate future settings and also to reduce visual noise and clutter.

How?

Refactor component structure and implement accordion drawer.

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Screen.Capture.on.2023-01-26.at.19-17-13.mp4

@getdave getdave self-assigned this Jan 21, 2023
@getdave getdave added Needs Design Feedback Needs general design feedback. [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Jan 21, 2023
@getdave
Copy link
Contributor Author

getdave commented Jan 21, 2023

This is very rough and still a PoC. We need to

  • refine the DOM structure/tab ordering.
  • fix the auto-commit behaviour of the Open in new tab (this is now very annoying).
  • fix the animation visuals so "drawer" contents cannot be seen through the toggle button during the animation.
  • visual refinements.

Help and assistance much appreciated 🙇

Questions

  • should the settings control appear when the link is not being edited?
  • should we remove the "auto commit" behaviour which happens when you toggle the Open in new tab toggle? This now feels unhelpful and unexpected.

@getdave getdave mentioned this pull request Jan 21, 2023
10 tasks
@github-actions
Copy link

github-actions bot commented Jan 21, 2023

Size Change: +442 B (0%)

Total Size: 1.31 MB

Filename Size Change
build/block-editor/index.min.js 192 kB +322 B (0%)
build/block-editor/style-rtl.css 14.4 kB +58 B (0%)
build/block-editor/style.css 14.4 kB +62 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.2 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.11 kB
build/block-editor/content.css 4.1 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
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 91 B
build/block-library/blocks/avatar/style.css 91 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 134 B
build/block-library/blocks/post-excerpt/style.css 134 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 458 B
build/block-library/blocks/query/editor.css 457 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 149 B
build/block-library/blocks/rss/editor.css 149 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 552 B
build/block-library/blocks/video/editor.css 555 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.11 kB
build/block-library/common.css 1.11 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.5 kB
build/block-library/editor.css 11.5 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 199 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.5 kB
build/block-library/style.css 12.5 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 51 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.8 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 8.09 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-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-post/index.min.js 34.5 kB
build/edit-post/style-rtl.css 7.5 kB
build/edit-post/style.css 7.5 kB
build/edit-site/index.min.js 64.1 kB
build/edit-site/style-rtl.css 9.68 kB
build/edit-site/style.css 9.67 kB
build/edit-widgets/index.min.js 16.9 kB
build/edit-widgets/style-rtl.css 4.52 kB
build/edit-widgets/style.css 4.52 kB
build/editor/index.min.js 45.4 kB
build/editor/style-rtl.css 3.58 kB
build/editor/style.css 3.57 kB
build/element/index.min.js 4.93 kB
build/escape-html/index.min.js 548 B
build/experiments/index.min.js 938 B
build/format-library/index.min.js 7.23 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.92 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.52 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.26 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

@jasmussen
Copy link
Contributor

Nice, I appreciate you included some basic animation as well:

link

I think that animation could be sped up a little, though. .2s seems good.

Should the actual drawer controls be below the settings icon? CC: @jameskoster

@jameskoster
Copy link
Contributor

Should the actual drawer controls be below the settings icon?

I considered it, but felt it a little strange to have interactive UI beneath the confirmation/dismissal actions:

Screenshot 2023-01-23 at 10 16 56

It might not be so bad in practise though, perhaps worth a try?

@getdave
Copy link
Contributor Author

getdave commented Jan 25, 2023

It might not be so bad in practise though, perhaps worth a try?

I can give this a go.

@getdave
Copy link
Contributor Author

getdave commented Jan 26, 2023

Screen.Capture.on.2023-01-26.at.19-17-13.mp4

I updated so that the drawer is below the button as per the design @jameskoster provided. The one issue with this is the tab order. I was speaking away from Github with @richtabor and we felt it should be

  • Search input
  • Settings toggle button
  • Settings fieldset (if "open")
  • Apply/Cancel buttons

To achieve this visual design whilst maintaining that tab order I've had to utilise the CSS order property. I know there are concerns about messing with the visual order so if we do go with this visual treatment then I'd like to get further input from a11y folks to ensure this doesn't cause any undue problems. That said, it seems to flow pretty well and the tab order could be argued to be both logical and inline with the visual presentation (depending on how you look at it).

I also sorted out the new borders.

@getdave
Copy link
Contributor Author

getdave commented Jan 26, 2023

I think that animation could be sped up a little, though. .2s seems good.

@jasmussen I changed this to 0.2s. It feels little janky I think because some of the inner element's layout causes layout to shift once the containers animation has completed. If you set the transition to something like 10 you will see what I mean.

@getdave
Copy link
Contributor Author

getdave commented Jan 30, 2023

You mentioned this in #47328, but definitely would be good to fix the tab sequence. In general, a toggle should be in the DOM before the controlled content. This makes a natural tab sequence of "open panel", tab, "editing things in panel". Right now, the user has to shift tab, which is less intuitive. It would really be preferable if the design supported a control that was located before the panel it opens.

@joedolson I have updated the tab ordering. It is now:

  • Search input
  • Settings toggle button (opens/closes the settings area).
  • Settings items (currently Text and Open in new tab).
  • Apply/Cancel buttons

I think this is what you were suggesting above? If you are able to confirm that would be greatly appreciated.

@getdave getdave marked this pull request as ready for review January 30, 2023 10:20
@getdave getdave requested a review from ellatrix as a code owner January 30, 2023 10:20
@scruffian
Copy link
Contributor

Under what situations does the "text" input appear?

@richtabor
Copy link
Member

Under what situations does the "text" input appear?

Editing an existing link.

@getdave
Copy link
Contributor Author

getdave commented Feb 2, 2023

@jasmussen @jameskoster I've:

  • updated the animation to be less janky
  • fixed spacing
  • fixed tab ordering
  • reset animation speed (probably best not to rely on the video to convey this)

Anything else you'd like me to fix up?

Screen.Capture.on.2023-02-02.at.14-47-32.mp4

cc @scruffian

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

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

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

@getdave
Copy link
Contributor Author

getdave commented Feb 3, 2023

@scruffian Thanks for the ✅. I found one UX question which is outstanding. What happens if you are not editing the link but you toggle the settings and then change the link text? How do you submit the changes?

We cannot rely on ENTER (power user move).

Should we hide the settings entirely unless you are editing the link? That way you can always click Apply to submit the changes.

IMHO it would be best to always require the link to be being "edited" in order to make any changes.

Aside: As a followup I'd like to make "open in new tab" require "Apply" as well.

Also curious to hear from @WordPress/gutenberg-design.

Screen Shot 2023-02-03 at 14 55 33

@richtabor
Copy link
Member

Should we hide the settings entirely unless you are editing the link?

Yes. Edit should be the entry to changing the values (and then you see Cancel/Apply.

@getdave
Copy link
Contributor Author

getdave commented Feb 3, 2023

I pushed an update which only shows settings controls if we are in "edit" mode.

I will need to add a test for this.

@richtabor
Copy link
Member

richtabor commented Feb 4, 2023

I pushed an update which only shows settings controls if we are in "edit" mode.

Feels much better now. It's clearer what action to take to edit the link, and to apply those changes.

Aside: As a followup I'd like to make "open in new tab" require "Apply" as well.

Agreed. The auto-closing when you toggle the control isn't a great interaction currently.

@getdave getdave requested a review from ntwb as a code owner February 6, 2023 06:57
@getdave getdave force-pushed the add/settings-drawer-to-link-control branch from f2dd331 to a88a3a9 Compare February 6, 2023 12:18
@getdave
Copy link
Contributor Author

getdave commented Feb 6, 2023

Waiting on e2e test fixes to go ✅

@getdave
Copy link
Contributor Author

getdave commented Feb 6, 2023

@richtabor Are you happy to 👍 on behalf of @WordPress/gutenberg-design?

@getdave
Copy link
Contributor Author

getdave commented Feb 6, 2023

@scruffian You still happy with your 👍 given the changes since you ✅ ?

@getdave getdave added [Type] Enhancement A suggestion for improvement. and removed Needs Design Feedback Needs general design feedback. labels Feb 6, 2023
@scruffian
Copy link
Contributor

Yes, still Looks good to me.

Copy link
Member

@richtabor richtabor left a comment

Choose a reason for hiding this comment

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

LGTM

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 6, 2023

Before:

Selecting words to become links:
Screenshot 2023-02-06 at 23 28 46

Adding the URL:
Screenshot 2023-02-06 at 23 32 56

Editing a link:
Screenshot 2023-02-06 at 23 42 44

After:

Selecting words to become links:
Screenshot 2023-02-06 at 23 29 30

Adding the URL:
Screenshot 2023-02-06 at 23 33 06

Editing a link:

Before clicking toggle link settings.
Screenshot 2023-02-06 at 23 43 11

After clicking toggle link settings.
Screenshot 2023-02-06 at 23 43 20

What I like with the After new approach is that the word is selected as it becomes obvious which words are selected as the link. I also like the obvious Cancel and Apply link/buttons. I do find it interesting that a new link toggle setting icon has been added. The only thing I miss from the new approach is having the Toggle link settings open by default showing the Open in new tab toggle.

Hmm I am not sure that link Text should be added into the Toggle link settings.
The placement as seen in the current implementation makes it much more visible.

All in all the link settings could add various less used link options, and not so much the more used ones. Even though the Open in new tab toggle is a part of it. There are probably other less used link settings people ask for that would be added into the link settings.

A close to home real life case.
Just think of creating a Core Editor Summary with all the links to issues. Every time we add a link we have to click the icon to open the Toggle link settings to click the toggle to turn on the Open in new tab. It becomes a two click operation. It gets kinda tiring after doing so after the first 7-8 links.

Another case.
Open in new tab is with the current implemented link setting is easy to notice. With the new approach open in new tab is hidden behind the link settings. Many users might wonder where it went, and will not even notice the new toggle setting icon. Keeping link settings open by default will make it obvious that it is still there.

@jdevalk
Copy link
Contributor

jdevalk commented Feb 7, 2023

Hi! How would a plugin (say... Yoast SEO) go about filtering the values in the drawer? If allowed that'd remove the need for Yoast SEO and other plugins like it to have its own version of the Link Control, which we'd much prefer.

@getdave
Copy link
Contributor Author

getdave commented Feb 7, 2023

Hi! How would a plugin (say... Yoast SEO) go about filtering the values in the drawer? If allowed that'd remove the need for Yoast SEO and other plugins like it to have its own version of the Link Control, which we'd much prefer.

Absolutely. I've been thinking about this a lot. Currently the answer is that there isn't a way but...

As you may be aware you can already passing in settings but there is no way for the values of those settings to be handled.

I'm aware of Plugins that need to extend Gutenberg's built in usages of Link Control. It's a tricky problem but I have a couple of ideas which I'm hoping to tackle soon. These would involve

  • creating a universal "update" function exported from LinkControl which consumers could employ to handle onChange in a standard fashion
  • providing a means for extenders to modify that function to handle processing the values of custom controls they add.

@getdave
Copy link
Contributor Author

getdave commented Feb 7, 2023

@paaljoachim Thanks for this thorough feedback. Much appreciated. I would defer to @jameskoster on much of it as he kindly undertook to provide the designs.

For what it's worth here's my take.

I can see that hiding Open in new tab behind the drawer seems like adding friction. However if we are going to add additional controls here it quickly becomes a necessity that we find a way to reduce the visual overload of having too many controls. This PR is a small step towards that as I'm trying to avoid tackling all the issues in one huge PR.

I'm in two minds about moving the Text control. The feedback I have received in the past is that the Text control is underused and just adds visual noise. Perhaps we can try "hiding" it for now and then seek feedback from users as to its impact on UX? I'm not convinced either way.

@paaljoachim
Copy link
Contributor

Hey Dave.
Let's say that the Toggle link settings will contain a few toggles. It would then be natural to include the Open in new tab in it.
Text control in the Toggle link settings I am hesitant about but we can try it out.
It would be great with a preference setting to where we can keep the Toggle link settings open by default.

@jdevalk
Copy link
Contributor

jdevalk commented Feb 7, 2023

@getdave yeah we'd need to be able to do more than just add in settings indeed. I think for our use case, you could even restrict what could be changed to href, rel and class? That might make it less prone to error?

@getdave getdave added the [Type] Feature New feature to highlight in changelogs. label Feb 7, 2023
@getdave
Copy link
Contributor Author

getdave commented Feb 7, 2023

@getdave yeah we'd need to be able to do more than just add in settings indeed. I think for our use case, you could even restrict what could be changed to href, rel and class? That might make it less prone to error?

@jdevalk I'd like to understand the requirements in more detail. I've raised this Issue to track it where the discussion can continue.

@getdave getdave merged commit d142cb0 into trunk Feb 7, 2023
@getdave getdave deleted the add/settings-drawer-to-link-control branch February 7, 2023 09:57
@jdevalk
Copy link
Contributor

jdevalk commented Feb 7, 2023

Thanks! I've asked the Yoast team to get involved there.

@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 7, 2023
@joedolson
Copy link
Contributor

Just noting that I'd consider friction against adding 'open in a new tab' to be a positive change, since from an accessibility standpoint, opening links in a new tab is almost always a bad thing.

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) Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement. [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants