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 API: Remove HTML source string normalization #10756

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 18, 2018

Related: #10678 (specifically https://github.com/WordPress/gutenberg/pull/10678/files#r225838572)

This pull request seeks to remove string normalization in the block factory method, which had been added in #10678 as a tolerance to avoid issues where an undefined value would not concatenate, relevant largely in merging the RichText values of two blocks.

The argument made is that while this provides some convenience, it does so inconsistently from the behavior of any other attribute source/type, where the block developer is otherwise responsible for assuring its specific value form before operating on it.

Implementation notes:

The process here was in auditing each block using a source: 'html' for any operations which assume a string shape (like the quote merge concatenation in #10678), ensuring the default value. This does not mean a default is provided for every occurrence of source: 'html', and as such no guarantee is made for safety of concatenating attributes of source: 'html'.

Changes also include adding the missing type for HTML-sourced attributes. I expect this may be a remnant of when the attribute was of the un-typed abstracted rich-text source.

Testing instructions:

Repeat testing instructions from #10678

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks labels Oct 18, 2018
@aduth aduth requested a review from ellatrix October 18, 2018 20:26
@aduth aduth requested a review from gziolo October 18, 2018 20:27
@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 18, 2018
@ktmn
Copy link

ktmn commented Oct 21, 2018

Changes also include adding the missing type for HTML-sourced attributes. I expect this may be a remnant of when the attribute was of the un-typed abstracted rich-text source.

Does this apply for rich text values that are stored in block comments as well?

If an old block is storing the rich text value as an array in the comments (when it's not possible to take the value from the HTML via a selector due to complexity/dynamic nature of the block):

<!-- wp:custom/block {"content":["Content"]} -->
<div class="wp-block-custom-block">Content</div>
<!-- /wp:custom/block -->

and you add the type: 'string' to the attribute it will invalidate the block.

Would it be possible to add backwards compatibility code that converts the array in the comments into a string rather than remove it since it doesn't match the type?

@aduth
Copy link
Member Author

aduth commented Oct 22, 2018

I should have been clearer in the description that the missing type was only added specifically for the core blocks which had been changed to use source: 'html', and not in the broader sense of any custom registered block.

If a block is updated to use the string shape of a RichText value and changing its attribute to source: 'html', it could use a deprecated migration to upgrade to the new variation. I'm not sure what's expected so far as deprecating the array form, but I might imagine such a deprecation process might include some sort of automated migration. Certainly not within scope of what's intended to be covered her.

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.

I did a series of block merges and evertything went well on my tests 👍 In #10678 an end 2 end tests was added so this change seems tested (manually & automatically ).

@@ -30,6 +30,7 @@ export const settings = {
attribute: 'src',
},
caption: {
type: 'string',
Copy link
Member

Choose a reason for hiding this comment

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

We have some docs with samples very similar to this.
https://github.com/WordPress/gutenberg/blob/master/docs/block-api/attributes.md#text

I think it may be worth it to add type: 'string', in these docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 9a6276b

@aduth aduth force-pushed the remove/block-factory-tolerance branch from 5474e83 to 956d97c Compare October 29, 2018 15:22
@aduth aduth force-pushed the remove/block-factory-tolerance branch from 956d97c to 9a6276b Compare October 29, 2018 15:27
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Sounds good to me too.

@aduth aduth merged commit d98a803 into master Oct 29, 2018
@aduth aduth deleted the remove/block-factory-tolerance branch October 29, 2018 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants