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

Multi block selection: more responsive UI #18915

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Multi block selection: more responsive UI #18915

merged 1 commit into from
Dec 6, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Dec 4, 2019 •
edited
Loading

Description

Alternative to #18836.

This PR makes the multi selection UI feel more responsive by instantly changing it when the selection changes.

How has this been tested?

Screenshots

Types of changes

Enhancement

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

@ellatrix ellatrix added [Feature] Block Multi Selection The ability to select and manipulate multiple blocks [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Enhancement A suggestion for improvement. labels Dec 4, 2019
@ellatrix ellatrix added this to the Gutenberg 7.1 milestone Dec 4, 2019
@ellatrix ellatrix mentioned this pull request Dec 4, 2019
Closed
6 tasks
@ellatrix ellatrix force-pushed the try/responsive-multi-select branch from afd723f to 1bede6e Compare December 5, 2019 07:44
@ellatrix ellatrix changed the base branch from fix/multi-select-nested to master December 5, 2019 07:44
@ellatrix ellatrix requested a review from jasmussen December 6, 2019 10:45
youknowriad
youknowriad reviewed Dec 6, 2019
View reviewed changes
packages/block-editor/src/components/block-list/index.js
@@ -68,7 +68,7 @@ class BlockList extends Component {

this.onSelectionStart = this.onSelectionStart.bind( this );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love if we can somehow isolate/decouple the multi-selection code form the BlockList rendering code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I feel like the PR converting to hooks is a good opportunity to do this.

youknowriad
youknowriad reviewed Dec 6, 2019
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Didn't check the code deeply, but this feels so great.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

Thanks!

@ellatrix ellatrix merged commit 4a1f3d7 into master Dec 6, 2019
@ellatrix ellatrix deleted the try/responsive-multi-select branch December 6, 2019 10:51
@mcsf
Copy link
Contributor

mcsf commented Dec 6, 2019

This feels very nice!

It does expose the following, though:

multi

Notice how selection terminates the moment the cursor returns to the initial block, even though I didn't release the pointer button.

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

select

@mcsf I can't reproduce. Which browser is this?

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

Ah, I do seem to run into issues with Firefox...

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

It works fine in Chrome and Safari. In Firefox, the whole paragraph gets selected after leaving the initial block, which indicates that Firefox is resetting the selection when content editable is switched off. Thinking what to do... 🤔

@ellatrix
Copy link
Member Author

ellatrix commented Dec 6, 2019

After testing the previous commit (bd8436f), it looks like Firefox didn't have any problems previously though.

@grappler grappler mentioned this pull request Dec 12, 2019
Closed
@jorgefilipecosta jorgefilipecosta mentioned this pull request Mar 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad approved these changes

@talldan talldan Awaiting requested review from talldan

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@daniloercoli daniloercoli Awaiting requested review from daniloercoli

@etoledom etoledom Awaiting requested review from etoledom

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

@Soean Soean Awaiting requested review from Soean

@TimothyBJacobs TimothyBJacobs Awaiting requested review from TimothyBJacobs

@jasmussen jasmussen Awaiting requested review from jasmussen

Assignees
No one assigned
Labels
[Feature] Block Multi Selection The ability to select and manipulate multiple blocks [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ellatrix @mcsf @youknowriad