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.

Sign up for GitHub

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

Jump to bottom

Reuse BlockControls in block navigation ellipsis menu #22462

Closed
wants to merge 6 commits into from
Closed

Reuse BlockControls in block navigation ellipsis menu #22462

wants to merge 6 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented May 19, 2020 •
edited
Loading

Description

This PR is a Proof of Concept of reusing the existing block controls from edit.js in the block navigation ellipsis menu.

This PR introduces the idea of <UniversalBlockControls /> (name to be changed) component that accepts controls groups and buttons, and knows how to apply them in different contexts. For the purposes of this Proof of Concept, it's the block toolbar and the navigation ellipsis menu. Maybe the could would be better if we could passed a prop with an object describing the menus we want to get in the end instead of children?

This is a part of a sequence of few PRs. I propose merging/reviewing these PRs in the following order:

1. #22427 - introduce very basic ellipsis menu to the block navigation

  1. Allow explicitly specifying additional block navigation menu options in block edit function #22463 - allow the block to define additional ellipsis menu items
  2. Allow explicitly specifying additional block navigation menu options in block edit function #22463 - introduce an abstraction that takes care of the toolbar items and ellipsis menu items at the same time

How has this been tested?

Screenshots

Zrzut ekranu 2020-05-19 o 15 45 48

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@adamziel adamziel self-assigned this May 19, 2020
@github-actions
Copy link

Size Change: +531 B (0%)

Total Size: 1.11 MB

Filename Size Change
build/block-editor/index.js 105 kB +505 B (0%)
build/block-library/index.js 119 kB +26 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 182 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 6.68 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.62 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.8 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.73 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@adamziel adamziel mentioned this pull request May 19, 2020
Closed
@adamziel adamziel added the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label May 19, 2020
@adamziel adamziel mentioned this pull request May 19, 2020
Merged
6 tasks
@talldan
Copy link
Contributor

talldan commented May 20, 2020

This looks like a really promising first iteration 👍 . There are a few challenges I noticed when testing.

One is that when choosing an option like Remove Block or Duplicate, focus is transferred outside of the block navigation to the block list itself. That seems like it would be confusing for those who rely on keyboard navigation and might want to perform further actions in the block navigation.

I think the block selection logic is built into the action that's dispatched when removing or duplicating, so I wonder what the best way to uncouple that would be.

There also seems to be an error thrown when opening the menu using the keyboard, something somewhere is unexpectedly undefined.

@adamziel
Copy link
Contributor Author

adamziel commented May 20, 2020 •
edited
Loading

@talldan all good notes 👍 I'll address the focus issue in a separate PR. Let's tackle it bit by bit, I propose the following sequence for the core changes:

#22427 - introduce very basic ellipsis menu to the block navigation
#22463 - allow the block to define additional ellipsis menu items
#22462 - introduce an abstraction that takes care of the toolbar items and ellipsis menu items at the same time

And spinning parallel PRs for improvements like focus or overflowing ellipsis button

Base automatically changed from add/ellipsis-menu to master May 21, 2020 08:47
@draganescu draganescu added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Jun 15, 2020
@adamziel
Copy link
Contributor Author

adamziel commented Jun 29, 2020 •
edited
Loading

The concept of the navigator ellipsis menu keeps evolving. This PR is stale and may not be needed after all. I'm closing it for now, and we can always revive it later after all.

@adamziel adamziel closed this Jun 29, 2020
@aristath aristath deleted the add/ellipsis-menu-options branch November 10, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talldan talldan Awaiting requested review from talldan

@draganescu draganescu Awaiting requested review from draganescu

Assignees

@adamziel adamziel

Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamziel @talldan @draganescu