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

Restore multi-select focus #3222

Closed
wants to merge 3 commits into from
Closed

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 29, 2017

Description

This PR restores setting focus on a multi-select action after multi-selecting. Somehow this was all reverted in a few commits after redoing BlockSettingsMenu.

A side effect of this is that the focus does not leave the Editable, which breaks deleting the multi-selection. Fixes #3191. See also #3194. Done in #3253.

How Has This Been Tested?

  1. Make a multi-selection.
  2. Observe the focus moving to the right block settings toggle.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@codecov
Copy link

codecov bot commented Oct 29, 2017

Codecov Report

Merging #3222 into master will increase coverage by 0.05%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3222      +/-   ##
=========================================
+ Coverage   31.24%   31.3%   +0.05%     
=========================================
  Files         222     222              
  Lines        6373    6380       +7     
  Branches     1136    1137       +1     
=========================================
+ Hits         1991    1997       +6     
- Misses       3679    3680       +1     
  Partials      703     703
Impacted Files Coverage Δ
editor/modes/visual-editor/block.js 0% <ø> (ø) ⬆️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
components/icon-button/index.js 100% <100%> (ø) ⬆️
components/button/index.js 90.9% <90.9%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5fc320...2b078ac. Read the comment docs.

@ellatrix ellatrix mentioned this pull request Oct 29, 2017
3 tasks
@ephox-mogran
Copy link
Contributor

I'm happy enough with this. My two concerns are:

a) whether we want to focus the button. My argument is presented in #3194. Essentially, it just boils down to I think it's odd that we would be able to modify the selection while our focus is on a button.
b) the focus here is being derived from the redux state. There might be a situation where the focus was actually on the block mover buttons, but something else changed so an action was fired, and therefore this would shift our focus because we "implicitly" link selecting with this button. What we want to do is implicitly link the first time we do a selection with this button. I'm not sure if that code would enforce this ... but maybe it does.

Other than that, I'm happy with this change.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Personally this seemed strange to me as a prop for button, since focusing is an explicit action. It might be a small naming thing (autoFocus? focusOnMount?). Also, we're not respecting changes to the prop over the lifecycle of the button (componentDidUpdate).

Still then it seems quite specific to the use-case of multi-selection and the block settings menu. Wondering if this should be handled by the componentDidMount and componentDidUpdate of BlockSettingsMenu to focus the button. This would mean that the button component would either need to expose its underlying DOM node or that we use findDOMNode to access the button by its ref, which are equally non-ideal.

@ellatrix
Copy link
Member Author

Also, we're not respecting changes to the prop over the lifecycle of the button (componentDidUpdate).

Correct, I guess it would be better to rename to autofocus or something similar.

@ellatrix
Copy link
Member Author

ellatrix commented Nov 2, 2017

Done via 3297.

@ellatrix ellatrix closed this Nov 2, 2017
@ellatrix ellatrix deleted the revert/multi-select-focus branch November 2, 2017 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants