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

[Navigation screen] Separate block navigator focus from the editor focus #22996

Merged
merged 12 commits into from
Jun 19, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jun 8, 2020

Description

Closes #22705

This PR is an attempt to separates the block navigator selection/focus from the editor area. Once applied, all actions performed in the navigator will only affect selection/focus within the navigator instead of trying to synchronize with the editor area:

2020-06-08 16-05-56 2020-06-08 16_06_55

CC @enriquesanchez and @karmatosed for sanity check

Editing the caption is not implemented yet.

Open questions:

  • Does it feel like the right approach?
  • The concept of selecting a navigator item, as in turning the icon dark once an item is clicked, does not seem to be useful anymore - should we get rid of it and switch to the caption edit mode on click?

How has this been tested?

  1. Enable the navigation experiment in Gutenberg > Experiments
  2. Go to Gutenberg > Navigation (beta) and create a menu
  3. Use the navigator using a keyboard, select/remove/duplicate some navigation items, confirm the behavior makes intuitive sense
  4. Go to the post editor, create a navigation block, select/move some navigation items using the navigator, confirm the behavior did not change.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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 Jun 8, 2020
@github-actions
Copy link

github-actions bot commented Jun 8, 2020

Size Change: +368 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/annotations/index.js 3.62 kB -2 B (0%)
build/api-fetch/index.js 3.4 kB -1 B
build/block-editor/index.js 107 kB +443 B (0%)
build/block-editor/style-rtl.css 10.7 kB -36 B (0%)
build/block-editor/style.css 10.7 kB -37 B (0%)
build/block-library/index.js 129 kB -1 B
build/blocks/index.js 48.1 kB +1 B
build/data/index.js 8.44 kB -1 B
build/edit-navigation/index.js 8.26 kB +4 B (0%)
build/edit-post/index.js 303 kB -2 B (0%)
build/edit-site/index.js 16.6 kB -3 B (0%)
build/keyboard-shortcuts/index.js 2.51 kB +2 B (0%)
build/nux/index.js 3.4 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 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 7.26 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-library/editor-rtl.css 7.86 kB 0 B
build/block-library/editor.css 7.87 kB 0 B
build/block-library/style-rtl.css 8 kB 0 B
build/block-library/style.css 8.01 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 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/components/index.js 196 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 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 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 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.72 kB 0 B
build/format-library/style-rtl.css 542 B 0 B
build/format-library/style.css 543 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/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 446 B 0 B
build/list-reusable-blocks/style.css 447 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/style-rtl.css 662 B 0 B
build/nux/style.css 657 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 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 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.06 kB 0 B
build/viewport/index.js 1.85 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 marked this pull request as ready for review June 8, 2020 14:16
@adamziel adamziel added [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Feature] Navigation Screen labels Jun 8, 2020
@adamziel
Copy link
Contributor Author

adamziel commented Jun 8, 2020

This PR breaks some tests ATM, I'd like to get some initial feedback on direction before debugging them.

@enriquesanchez
Copy link
Contributor

Hi @adamziel 👋

This feels good to me. Focus placement is more obvious to me now.

@talldan
Copy link
Contributor

talldan commented Jun 11, 2020

I agree with @enriquesanchez that is feels a lot more reliable and obvious where focus will end up. 👍

I'm not sure what to do about the selected block being highlighted, potentially it's not needed any more, although at the same time it might be useful for a user looking to jump between the normal editor and the block navigation. Though it could also be confusing.

I notice we're showing the movers and settings menu for the selected block, which is probably not needed now that you can't select the block in the block navigation. I think they should be shown for the focused and hovered blocks only.

I still think an option to jump to the actual block from the block navigation might be useful, I wonder if we might want to explore a 'Go To ...' option in the settings menu as part of a separate issue?

@draganescu
Copy link
Contributor

I just want to +1 the "Go to..." idea!
Options for the copy:

  • Select in menu
  • Find in menu
  • Go to item
  • Show item

@enriquesanchez
Copy link
Contributor

I'm not sure what to do about the selected block being highlighted, potentially it's not needed any more, although at the same time it might be useful for a user looking to jump between the normal editor and the block navigation. Though it could also be confusing.

I was actually confused and did not know that the block icon was meant to take you to the nav block. I don't think that's obvious, we certainly don't have that same interaction anywhere else, do we?

I still think an option to jump to the actual block from the block navigation might be useful, I wonder if we might want to explore a 'Go To ...' option in the settings menu as part of a separate issue?

+1 to this idea, but I'm finding it hard to come up with the right text for this. In addition to Andrei's suggestions, I'll add "Go to block"

@adamziel
Copy link
Contributor Author

adamziel commented Jun 12, 2020

All good notes! I removed the dark CSS when an item is "selected" and added a "Go to" action (naming TBD):

Zrzut ekranu 2020-06-12 o 17 55 32

The next step here for me is to fix unit/e2e tests and rigorously user-test the entire PR.

@draganescu
Copy link
Contributor

I love that Go to option! Feels pretty neat :)
I tested this locally and in Firefox it seems that some of the clicks did not register correctly:

weird focus

@adamziel
Copy link
Contributor Author

@draganescu good catch! I just fixed the non-responding "Go to block" button. I am unable to reproduce the other issue (non-responding navigator item). Let's get these tests green and see if it's still happening.

@adamziel
Copy link
Contributor Author

Alright, this one seems to be ready for re-review!

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.

I tested this locally and it works to keep the selection in the navigator for the navigation screen while not affecting how selection works in the post editor.

There is a spinoff issue #23269 that should improve how returning the focus to the block list works, as right now a nested block doesn't get it's editable area focused.

@adamziel adamziel merged commit 4b9fc09 into master Jun 19, 2020
@adamziel adamziel deleted the fix/navigator-focus branch June 19, 2020 16:55
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 19, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigator iterations: Selection and focus
4 participants