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

Fix odd behavior when transforming Text to List block #2365

Merged
merged 4 commits into from
May 22, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented May 21, 2019

Fixes #2304.

This PR removes the prefixed transformation from the list block, not allowing the Text block to convert into a list block automatically.

@googlebot googlebot added the cla: yes Signed the Google CLA label May 21, 2019
@miina
Copy link
Contributor Author

miina commented May 21, 2019

@swissspidy Do you have any ideas here how to still keep the transformations but include the attributes from the Text block?

I was originally thinking of blocks.switchToBlockType.transformedBlock but apparently, that's not applied in case of automatic transformation.

Also tried to change the prefixed list transformations to include getting attributes but unfortunately the origin block isn't passed in as a param to transform, just content is passed, as far as I understood.

Right now this PR removes the prefixed transformations for list so that it doesn't convert automatically, however, not sure if there's an easy way how to keep that, too. Would be great if the transformation filter would be applied in case of automatic transformations, too. Maybe it would make sense to resolve this upstream instead (Edit: by adding this filter for automatic transformation, too).

But perhaps you have another simple solution in mind that I missed. Alternatively, we could remove the transformations for now.

Thoughts?

@westonruter westonruter changed the base branch from amp-stories-redux to develop May 21, 2019 23:16
@miina
Copy link
Contributor Author

miina commented May 22, 2019

The list block doesn't allow font/color changes so probably, for now, it's better to just remove the automated prefixed transformations, otherwise, the user won't have a choice to customize numbered/bulleted sentences since they'd automatically convert to a list instead.

@miina miina marked this pull request as ready for review May 22, 2019 09:43
@miina miina requested a review from swissspidy May 22, 2019 09:43
@swissspidy swissspidy added this to the v1.2 milestone May 22, 2019
@swissspidy
Copy link
Collaborator

Do you have any ideas here how to still keep the transformations but include the attributes from the Text block?

Not really at this moment. 😕

Would be great if the transformation filter would be applied in case of automatic transformations, too. Maybe it would make sense to resolve this upstream instead

Opening an issue or even PR upstream would be good 👍

The list block doesn't allow font/color changes so probably, for now, it's better to just remove the automated prefixed transformations

Hmm because the list blocks only has transforms form and to the default paragraph block, maybe we should add some transforms to our custom text block.

Specifically, these two:

https://github.com/WordPress/gutenberg/blob/f2cafbd39ed0e905980e6ed2d151e0747aa72151/packages/block-library/src/list/transforms.js#L37-L59
https://github.com/WordPress/gutenberg/blob/f2cafbd39ed0e905980e6ed2d151e0747aa72151/packages/block-library/src/list/transforms.js#L110-L124

Could be something for a separate issue / PR where we could review all blocks and their transforms.

Thoughts?

@miina
Copy link
Contributor Author

miina commented May 22, 2019

Hmm because the list blocks only has transforms form and to the default paragraph block, maybe we should add some transforms to our custom text block.

Good point, we're not using the paragraph block anyway so we could replace these transformations. Makes sense in a separate PR, this one is for the bug fix specifically.

@swissspidy
Copy link
Collaborator

Cool, created #2380 for that.

@miina miina merged commit 086ced5 into develop May 22, 2019
@miina miina deleted the amp-story/2304-fix_jumping_after_transform branch May 22, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Odd Behavior when typing a numbered list into a text block
4 participants