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

Block Editor: Maintain selection when multi-selection toggled #16336

Closed
wants to merge 3 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 27, 2019

Extracted from #15927
Previously: #14640 (comment)

This pull request seeks to revise the behavior of the selection reducer to persist selected block through (multi-)selection toggling. Without these changes, an image will become unselected when resizing it, or as in #15927, the Columns block would become unselected when resizing columns.

Testing Instructions:

Verify that resizing an image does not unselect the block.

  1. Navigate to Posts > Add New
  2. Insert an image block
  3. Select an image from the media library
  4. Resize the image
  5. Note that the image block is still selected

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] Block Multi Selection The ability to select and manipulate multiple blocks labels Jun 27, 2019
@aduth aduth requested a review from ellatrix June 27, 2019 18:23
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Worked well on my tests and the change makes sense 👍

@aduth
Copy link
Member Author

aduth commented Jul 1, 2019

@jorgefilipecosta In some final review of the changes, I was looking at the initial state object and it occurred to me that if the intent with "toggling" selection is to allow/disallow multi-selection, we should cancel any multi-selection (isMultiSelecting) when disallowing. I've added this in 0be9fea.

@@ -699,6 +699,27 @@ export function isCaretWithinFormattedText( state = false, action ) {
}

const BLOCK_SELECTION_EMPTY_OBJECT = {};
Copy link
Member Author

Choose a reason for hiding this comment

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

A few notes on my documentation attempts here (cc @ellatrix):

  • I assumed BLOCK_SELECTION_EMPTY_OBJECT was used here to allow for some strict equality checks in selectors, though I could not find any place we actually do this. The tests also pass if its use below in BLOCK_SELECTION_INITIAL_STATE are replaced with new objects. Can you clarify what purpose this value serves?
  • And on that note, for what reason does it need to be an object? I had planned to document start and end as an object, but could only find that we reference clientId on it. Are there other properties, or could it just be replaced with a direct reference to the clientId?

Copy link
Member

Choose a reason for hiding this comment

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

Don't we do any strict equality checks in the reducer? Perhaps I removed that later on, so this wouldn't be needed any longer.

The selection object can also include the rich text identifier and caret position, if a rich text instance is selected.

@paaljoachim
Copy link
Contributor

@jorgefilipecosta @aduth What is still needed to get this merged?

@youknowriad
Copy link
Contributor

Up again here :) should we close or land this PR?

@aduth
Copy link
Member Author

aduth commented Jan 29, 2020

It appears this is no longer an issue. I cannot reproduce the original steps.

I suspect it was fixed separately as a result of #17467.

@aduth aduth closed this Jan 29, 2020
@aduth aduth deleted the update/maintain-selection-toggle-select branch January 29, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Multi Selection The ability to select and manipulate multiple blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants