Fix issue where resetBlocks can result in an incorrect block selection#21598
Merged
Fix issue where resetBlocks can result in an incorrect block selection#21598
Conversation
|
Size Change: +487 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
Contributor
|
@dan I tested this and it works perfectly! It solves the bug and the code looks like a good improvement. Why is this still a draft? |
youknowriad
reviewed
Jun 23, 2020
| // RESET_BLOCKS, clear the block selection state. | ||
| if ( ! selectionStartBlock || ! selectionEndBlock ) { | ||
| yield clearSelectedBlock(); | ||
| } |
Contributor
There was a problem hiding this comment.
Do you think it'd be better to solve this with a Higher-order reducer instead to avoid two dispatches?
Contributor
Author
There was a problem hiding this comment.
@youknowriad Initially looked into making it part of the reducer using the selection helper that's already there (#21371). The trouble I had is that selectionStart and selectionEnd are two separate reducers, but the code needs to access both bits of state to determine if selection should be cleared.
Not sure if that's something that can be solved at reducer level without combining the two reducers.
Contributor
There was a problem hiding this comment.
@talldan yes it can we have a few instances of this withBlockReset is one example.
Contributor
Author
There was a problem hiding this comment.
Ah, ok great, will take a look 👍
Contributor
Author
|
@draganescu I forgot about this one a little bit! I need to look at why the tests are failing really before it's ready for review. |
1bdbcc4 to
8f7044b
Compare
Contributor
Author
|
I've updated this to use a higher order reducer as suggested. It's quite a big diff now as the |
Make resetBlocks a generator action which also clears selection if required Add tests and modify action to return earlier if possible Fix typo in test description Make selectionStart, selectionEnd a combined reducer WIP Update selectors and effects to use selection object in state Add JSDoc for higher-order reducer Add tests for RESET_BLOCKS Revert RESET_BLOCKS action test changes
e900efa to
0e33c42
Compare
youknowriad
reviewed
Jan 14, 2021
| * | ||
| * @return {Object} Updated state. | ||
| */ | ||
| selectionStart( state = {}, action ) { |
Contributor
There was a problem hiding this comment.
Do you think it should be just "start" and "end" to avoid the redundancy
Contributor
Author
There was a problem hiding this comment.
@youknowriad I've tried that out in ead6d1a. It also allowed for refactoring the higher-order reducer into the main selection reducer.
I don't think it resulted in much reduced repetition, but I like that the code is a little easier to follow now that there's no higher order reducer.
Thanks for the review btw.
youknowriad
approved these changes
Jan 14, 2021
Contributor
youknowriad
left a comment
There was a problem hiding this comment.
Left a small comment but the code looks good.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #21445. Alternative to #21371. Bug discovered while working on #21340.
When RESET_BLOCKS is dispatched and results in the selected blocks being replaced, selection state in the store is not cleared, which can cause the top toolbar to be shown for a block that no longer exists.
This PR converts the resetBlocks action into a generator action, and adds additional logic for resetting selection when needed.This PR adds a higher order reducer for selection that updates the selection state as necessary when RESET_BLOCKS is dispatched.
How has this been tested?
Unit tests added.
Steps to reproduce:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: