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

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 25, 2022

Screen Shot 2022-02-01 at 10 47 41

Description

In #36977 (comment) we learnt that some users want to be able to revert the submenu block to a standard link. The expectation was/is that deleting the submenu would restore the link but of course as the block is in fact a submenu block this just removes the entire block.

This PR seeks to implement one possible solution which is to show an explicit "Revert to Link" button in the block toolbar. This effectively just does the same thing as the block's built-in "Transform" option, but it makes it more explicit for this specific use case.

I'm no designer so this is just one idea for solving this case and we will also no doubt need a better icon choice.

Curious to hear from those more experienced with this block as well.

Also would love feedback from @courtneyr-dev who originally noticed the quirk and requested the change.

How has this been tested?

  1. Add Nav block.
  2. Create items.
  3. Add a submenu to one of the items and add some sub items.
  4. On the parent submenu item, click the toolbar button to revert to a standard link.
  5. The link details should all be retained except from any child blocks which will be disguarded.

⚠️ As noted above, any children of the submenu block being revert will be lost. Is that the behaviour we want? If not then what should happen?

Screenshots

Screen.Capture.on.2022-01-31.at.10-57-20.mp4

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@getdave getdave added [Type] Enhancement A suggestion for improvement. [Block] Submenu Affects the Submenu Block - for submenus in navigation labels Jan 25, 2022
@getdave getdave self-assigned this Jan 25, 2022
@getdave getdave marked this pull request as ready for review January 25, 2022 10:11
@getdave getdave requested a review from talldan January 25, 2022 10:12
@getdave getdave added [Block] Navigation Affects the Navigation Block and removed [Block] Navigation Affects the Navigation Block labels Jan 25, 2022
@github-actions
Copy link

github-actions bot commented Jan 25, 2022

Size Change: +1.64 kB (0%)

Total Size: 1.15 MB

Filename Size Change
build/block-editor/index.min.js 142 kB +12 B (0%)
build/block-library/blocks/cover/style-rtl.css 1.55 kB +155 B (+11%) ⚠️
build/block-library/blocks/cover/style.css 1.55 kB +155 B (+11%) ⚠️
build/block-library/blocks/separator/style-rtl.css 233 B -12 B (-5%)
build/block-library/blocks/separator/style.css 233 B -12 B (-5%)
build/block-library/editor-rtl.css 10.1 kB +44 B (0%)
build/block-library/editor.css 10.1 kB +44 B (0%)
build/block-library/index.min.js 168 kB +749 B (0%)
build/block-library/style-rtl.css 11.3 kB +194 B (+2%)
build/block-library/style.css 11.3 kB +190 B (+2%)
build/components/index.min.js 215 kB +72 B (0%)
build/edit-post/index.min.js 29.9 kB +62 B (0%)
build/editor/index.min.js 38.4 kB -11 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.22 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/style-rtl.css 14.8 kB
build/block-editor/style.css 14.8 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 65 B
build/block-library/blocks/archives/style.css 65 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 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 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 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 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-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 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-query-loop/editor-rtl.css 95 B
build/block-library/blocks/comments-query-loop/editor.css 95 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
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 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 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 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 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 965 B
build/block-library/blocks/gallery/editor.css 967 B
build/block-library/blocks/gallery/style-rtl.css 1.61 kB
build/block-library/blocks/gallery/style.css 1.61 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 159 B
build/block-library/blocks/group/editor.css 159 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 114 B
build/block-library/blocks/heading/style.css 114 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 810 B
build/block-library/blocks/image/editor.css 809 B
build/block-library/blocks/image/style-rtl.css 500 B
build/block-library/blocks/image/style.css 503 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 199 B
build/block-library/blocks/latest-posts/editor.css 198 B
build/block-library/blocks/latest-posts/style-rtl.css 447 B
build/block-library/blocks/latest-posts/style.css 446 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 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 493 B
build/block-library/blocks/media-text/style.css 490 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 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 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-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.98 kB
build/block-library/blocks/navigation/editor.css 1.99 kB
build/block-library/blocks/navigation/style-rtl.css 1.85 kB
build/block-library/blocks/navigation/style.css 1.84 kB
build/block-library/blocks/navigation/view.min.js 2.81 kB
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 401 B
build/block-library/blocks/page-list/editor.css 402 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 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 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/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 521 B
build/block-library/blocks/post-comments/style.css 521 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 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/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 323 B
build/block-library/blocks/post-template/style.css 323 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 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 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 389 B
build/block-library/blocks/pullquote/style.css 388 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 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 201 B
build/block-library/blocks/quote/style.css 201 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 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 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 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 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 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 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.37 kB
build/block-library/blocks/social-links/style.css 1.36 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 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 214 B
build/block-library/blocks/tag-cloud/style.css 215 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 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 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 921 B
build/block-library/common.css 919 B
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 676 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.4 kB
build/components/style-rtl.css 15.5 kB
build/components/style.css 15.5 kB
build/compose/index.min.js 11.2 kB
build/core-data/index.min.js 13.4 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.44 kB
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16.2 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/style-rtl.css 7.17 kB
build/edit-post/style.css 7.16 kB
build/edit-site/index.min.js 41.6 kB
build/edit-site/style-rtl.css 7.22 kB
build/edit-site/style.css 7.21 kB
build/edit-widgets/index.min.js 16.7 kB
build/edit-widgets/style-rtl.css 4.17 kB
build/edit-widgets/style.css 4.17 kB
build/editor/style-rtl.css 3.71 kB
build/editor/style.css 3.71 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.58 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.75 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.72 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.09 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.95 kB
build/primitives/index.min.js 917 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 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.65 kB
build/reusable-blocks/index.min.js 2.22 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11 kB
build/server-side-render/index.min.js 1.58 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@getdave getdave mentioned this pull request Jan 26, 2022
38 tasks
@getdave getdave added the Needs Design Feedback Needs general design feedback. label Jan 27, 2022
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

I'm hesitant to enable this without showing a warning when there are items in the submenu. As it is, it's too easy to delete a whole submenu by clicking on a button with an icon that looks like a simple "undo" action.

Even if we show the text label, it might be confusing, particularly if "Open on click" is enabled (users might think they're just converting the top-level item back into a link instead of getting rid of the whole submenu).

@getdave
Copy link
Contributor Author

getdave commented Jan 28, 2022

I recognise the concerns expressed here. That said, we have a clear use case where users are being confused by the continued presence of a submenu block even when there are no submenu items. I'd really like to find a way to smooth out that path.

I appreciate your help.

I'm hesitant to enable this without showing a warning when there are items in the submenu.

That's a fair point. Would adding a confirmation dialogue if there are sub items resolve this concern for you? If not is there anything else you can think of that would?

@courtneyr-dev
Copy link
Contributor

Users are being confused by the continued presence of a submenu block even when there are no submenu items.

I concur. If users leave the parent menu as having a submenu when none are present, website visitors would also be confused seeing the drop-down with no submenu items present.

I am not the best yet with code, but the idea would be, if there is 1+, then show dropdown, else show standard link.

@TimothyBJacobs
Copy link
Member

Is it feasible to make deleting a block function as the users expect? That has the advantages of users not needing to learn another pattern for destructive actions.

I think it'd also be generally useful as well to allow for blocks to provide an onDelete handler of some kind.

@talldan
Copy link
Contributor

talldan commented Jan 31, 2022

Is it feasible to make deleting a block function as the users expect? That has the advantages of users not needing to learn another pattern for destructive actions.

@TimothyBJacobs Would you be able to go into more detail about this?

I'm hesitant to enable this without showing a warning when there are items in the submenu.

It could also be that the button only appears when the submenu is empty so that there's less risk.

I think there's also the possibility for more natural in-canvas options for add/remove submenus. Like a + button below a link to create a submenu (it could work like the in-between inserter). Similarly an x button in the same place to remove an empty submenu. This is just a random idea though, and it could do with some design exploration.

@getdave
Copy link
Contributor Author

getdave commented Jan 31, 2022

It could also be that the button only appears when the submenu is empty so that there's less risk.

I think that's a good simple solution @talldan. Thank you for the clear insight.

Is it feasible to make deleting a block function as the users expect? That has the advantages of users not needing to learn another pattern for destructive actions.

I don't think it's possible at this time and, whilst I understand the motivation, I'm also not convinced it's something we should be doing. The delete action always performs the same behaviour across all blocks - removing the block. If we start making exceptions we could end up with a fragmented and confusing user experience.

I would still vote for a clear button dedicated to this edge case (as per this PR). I'm going to refine the PR today to include some of the suggestions included here whilst avoiding any "magic" behaviour (i.e. things happening automatically based on assumptions about user intent).

I hope you don't mind me adding a few of you as reviewers on the PR? I'll ping you when it's ready.

Thanks again for all the feedback and input here. It's really productive and useful 🙇‍♂️

@getdave
Copy link
Contributor Author

getdave commented Jan 31, 2022

Ok @talldan @courtneyr-dev @TimothyBJacobs I've now changed the code so that the button only appears is there are no items in the submenu. That feels like a healthy compromise.

I've updated the design to use the undo icon but I'd love to hear from @javierarce on any suggested alternatives to this.

@draganescu
Copy link
Contributor

Random thought: what if instead of yet-another-button we move to do things preemptively:

  • add a warning to the block's inspector, like "you have empty submenus" and an action to "Convert empty submenus to links"
  • automatically convert a submenu (a la autocorrect) to a link when the last child of the submenu is removed. This I think is unobtrusive and easily reverted by the user because adding a child converts the link to a submenu again.

On the PR in particular, the icon absolutely has to be changed. I do think that showin that button only when the submenu is empty is a great idea, but my suggestions above are an attempt at a more fluid experience.

@getdave
Copy link
Contributor Author

getdave commented Jan 31, 2022

  • add a warning to the block's inspector, like "you have empty submenus" and an action to "Convert empty submenus to

That's another option I'd not considered. More "hidden" though.

  • automatically convert a submenu (a la autocorrect) to a link when the last child of the submenu is removed. This I think is unobtrusive and easily reverted by the user because adding a child converts the link to a submenu again.

Some prior discussion on this here. There was no clear consensus but in general I dislike "magic" in UIs. Generally I prefer clear unambiguous controls that respond to user action. Automatically converting the submenu back to a link might also be a confusing experience, but I'm more than open to alternatives - I might even open a PR for comparison...perhaps that will be best!

On the PR in particular, the icon absolutely has to be changed.

I agree as I mentioned in the description 😄

I do think that showin that button only when the submenu is empty is a great idea...

Kudos to @talldan for that one.

...but my suggestions above are an attempt at a more fluid experience.

I recognise this. I still have concerns about having things happen "by magic" but I'd like to explore this direction as well.

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2022

My ideal option would be what @draganescu mentioned:

@adamziel I responded to that in this comment. Hopefully that explains my reasoning around at least trialing this more explicit approach as a first option.

@getdave getdave requested a review from adamziel February 9, 2022 11:38
@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2022

Assuming the e2e tests pass on CI I'm now seeking a technical ✅ on this PR.

@@ -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.

@adamziel
Copy link
Contributor

adamziel commented Feb 9, 2022

It tests well, thank you @getdave! That being said, I have a UX concern: As a user I am confused why is the "revert" button sometimes not there. Could we always display it but make it inactive with an alt text explaining the situation when the submenu isn't "empty"?

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2022

It tests well, thank you @getdave! That being said, I have a UX concern: As a user I am confused why is the "revert" button sometimes not there. Could we always display it but make it inactive with an alt text explaining the situation when the submenu isn't "empty"?

Good question but that's one I'd prefer to defer to an expert such as @javierarce.

Could we always display it but make it inactive with an alt text explaining the situation when the submenu isn't "empty"?

Also one I'd appreciate @javierarce's opinion on. @adamziel are there any other examples of this pattern in Gutenberg right now? I can't think of one.

@adamziel
Copy link
Contributor

adamziel commented Feb 9, 2022

@getdave I'm not sure! I'm just biased towards not hiding UI elements without an explanation – otherwise I find myself thinking wait, there was a feature here, hmm... I guess they removed it.

@javierarce
Copy link
Contributor

It tests well, thank you @getdave! That being said, I have a UX concern: As a user I am confused why is the "revert" button sometimes not there. Could we always display it but make it inactive with an alt text explaining the situation when the submenu isn't "empty"?

I like @adamziel's idea. It will probably help reduce the frustration.

Also one I'd appreciate @javierarce's opinion on. @adamziel are there any other examples of this pattern in Gutenberg right now? I can't think of one.

I don't remember examples that are identical… the closest thing could be these disabled buttons in the list toolbar.

world local_wp-admin_post php_post=1 action=edit

@getdave
Copy link
Contributor Author

getdave commented Feb 10, 2022

Next step: add disabled button and update e2e tests.

@getdave
Copy link
Contributor Author

getdave commented Feb 10, 2022

Screen Shot 2022-02-10 at 13 19 43

@javierarce @adamziel I've updated with the disabled state.

If you're happy with the result (and the code) I'm seeking a ✅

@javierarce
Copy link
Contributor

If you're happy with the result (and the code) I'm seeking a ✅

Looks good to me!

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Codewise this LGTM 🚢

@getdave getdave merged commit 433ee19 into trunk Feb 11, 2022
@getdave getdave deleted the try/explicit-revert-to-submenu-block branch February 11, 2022 13:19
@github-actions github-actions bot added this to the Gutenberg 12.7 milestone Feb 11, 2022
@cbravobernal cbravobernal changed the title Add revert button to submenu block Submenu: Add revert button. Feb 25, 2022
@cbravobernal cbravobernal added the [Package] Block library /packages/block-library label Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Submenu Affects the Submenu Block - for submenus in navigation Needs Design Feedback Needs general design feedback. [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.