Skip to content

Conversation

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Mar 5, 2018

Description

Fixes: #5369
This PR fixes a bug where it was impossible to delete blocks after transforming then.

The bug was in the blockOrder reducer. It was assuming that the replaced blocks have different UID's when compared to the original block. That is not the case and the UID's of the replaced blocks can be the same as the original block, so the logic of the reducer was corrected.

How Has This Been Tested?

Add a paragraph, add some text and transform it into a list. Remove the list and verify everything worked as expected.
Do the same but nesting the paragraph inside a collumn.

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Mar 5, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Mar 5, 2018
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.

Code-wise this looks good.

But it begs a question: Should we want to accommodate the behavior where replacement reuses an existing block UID? This seems easy to overlook in future maintenance, and I suspect it shouldn't be too difficult to generate a new UID for the transformed block.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Mar 5, 2018

Hi @aduth that is right, generating different uids should be simple, but I am almost certain that the uids being generated were different and just recently become equal. So I thought that they were changed because of some reason.
The fact that uids are the same looks very intentional and there is a comment saying:
// The first transformed block whose type matches the "destination"
// type gets to keep the existing UID of the first block..

@aduth
Copy link
Member

aduth commented Mar 5, 2018

Added in #484. @youknowriad , do you recall if there was a specific reason behind the desire to reuse a UID in replacing the block?

Just trying to explore which path will end up having the best long-term maintainability..

@youknowriad
Copy link
Contributor

@aduth I think my thinking at that moment was related to the fact we use uid as key prop when rendering the block loop, this avoids mounting/unmounting, but I don't recall if generating a new uid creates an issue or not.

@jorgefilipecosta
Copy link
Member Author

I expected it was related to select or scroll behavior, maybe we can give it a try and see if there are no undesirable effects.

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.

I'm finding that this resolves another issue in master where merging blocks in a nested context, then deleting the nested parent results in an error:

  1. Add Columns block
  2. Insert text into the second column
  3. Backspace until second column text is deleted / merged
  4. Remove the Columns block

In this branch, it works. In master, it errors.

It's a similar issue of UIDs being clobbered from state even though they should remain.

I'd be inclined to merge this as-is 👍

@aduth aduth added this to the 2.4 milestone Mar 8, 2018
@jorgefilipecosta jorgefilipecosta merged commit fd3429b into master Mar 8, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/block-cannot-be-removed-after-transform branch March 8, 2018 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Block cannot be removed after turned into another block type

4 participants