Omit serializing block attributes that match any default attribute values #1910

westonruter · 2017-07-15T05:52:43Z

Before the patch, the Text block that gets dropCap toggled from true to false gets serialized as:

With the changes, it gets serialized as (note the absence of the default value among the serialized attributes), and the fixed class in the Text block:

When dropCap is toggled back on, it gets serialized to:

westonruter · 2017-07-25T20:36:05Z

Rebased. d14e0b5 rewritten to 1b10e54.

westonruter · 2017-07-27T19:25:35Z

Working on resolving the conflicts…

aduth · 2017-07-27T19:36:17Z

I'd been waiting to review this pending #1905, but I think it might be good to get this in and I can rebase #1905 separately. Let me know when conflicts are resolved and I'll take a look.

westonruter · 2017-07-27T20:38:00Z

@aduth before I work on fixing the conflict, does this change get a 👍?

aduth

Seems like a good idea.

aduth · 2017-07-27T20:43:44Z

blocks/api/serializer.js

+
+	// Remove attributes that are the same as the defaults.
+	if ( blockType.defaultAttributes ) {
+		saveAttributes = pickBy( saveAttributes, ( value, key ) => ! isEqual( value, blockType.defaultAttributes[ key ] ) );


I don't think we need (or want) this to be a deep equality isEqual, but rather just strict equality !==

What about when an attribute contain an object? Then this will fail !== matching.

In other words, value !== blockType.defaultAttributes[ key ] will always be true for objects.

@aduth 👍?

In other words, value !== blockType.defaultAttributes[ key ] will always be true for objects.

Well, not necessarily; not if the attribute value is never updated. But it's a fair point that if they are updated, and their nested structure matches the default value, it should probably be omitted all the same. I'm fine with isEqual.

timmyc

I gave this branch a spin too and it seems like a great change. One minor comment.

timmyc · 2017-07-27T17:00:43Z

blocks/library/text/index.js

@@ -108,7 +112,7 @@ registerBlockType( 'core/text', {

 	save( { attributes } ) {
 		const { align, content, dropCap } = attributes;
-		const className = dropCap && 'has-drop-cap';
+		const className = dropCap ? 'has-drop-cap' : null;


Any reason to not just do this inline like return <p className={ dropCap ? 'has-drop-cap' : null }

No reason, other than it was using a className var to begin with.

westonruter · 2017-07-27T23:44:06Z

Rebased. 1b10e54 rewritten as 03e92f1.

codecov-io · 2017-07-27T23:47:10Z

Codecov Report

Merging #1910 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1910      +/-   ##
==========================================
+ Coverage   20.21%   20.25%   +0.03%     
==========================================
  Files         135      135              
  Lines        4225     4227       +2     
  Branches      717      718       +1     
==========================================
+ Hits          854      856       +2     
  Misses       2842     2842              
  Partials      529      529

Impacted Files	Coverage Δ
blocks/library/text/index.js	`41.17% <100%> (ø)`	⬆️
blocks/api/serializer.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6c3a4f...d6c8fd4. Read the comment docs.

aduth · 2017-07-28T17:37:14Z

blocks/api/serializer.js

+
+	// Remove attributes that are the same as the defaults.
+	if ( blockType.defaultAttributes ) {
+		saveAttributes = pickBy( saveAttributes, ( value, key ) => ! isEqual( value, blockType.defaultAttributes[ key ] ) );


In other words, value !== blockType.defaultAttributes[ key ] will always be true for objects.

Well, not necessarily; not if the attribute value is never updated. But it's a fair point that if they are updated, and their nested structure matches the default value, it should probably be omitted all the same. I'm fine with isEqual.

aduth · 2017-07-28T17:39:00Z

blocks/api/test/serializer.js

@@ -195,13 +199,14 @@ describe( 'block serializer', () => {
 				{
 					name: 'core/test-block',
 					attributes: {
+						foo: false,


One of two things should be done here:

We should set a bar: false value in the attributes here. As is we're not really testing that the default value is omitted, because the attribute isn't set anyways.

We should use createBlock to construct the block object and rely on its behavior to set the defaults into attributes.

My preference is toward the latter.

@aduth thanks for the feedback. Done in 71a07fc9bac6348a28545ad9cfe23e91c6c50a26.

aduth

By failing test, it appears we need to update markup for cover image fixture.

Otherwise LGTM 👍

westonruter · 2017-07-29T02:59:44Z

Thanks, I amended that cover-image block fix onto d272cbd.

Fixed failing test.

westonruter mentioned this pull request Jul 15, 2017

Dropcap tag doesn't get cleaned up #1768

Closed

aduth self-requested a review July 20, 2017 12:54

westonruter force-pushed the fix/omit-serializing-default-attributes branch from d14e0b5 to 1b10e54 Compare July 25, 2017 20:35

aduth reviewed Jul 27, 2017

View reviewed changes

timmyc reviewed Jul 27, 2017

View reviewed changes

westonruter force-pushed the fix/omit-serializing-default-attributes branch from 1b10e54 to 03e92f1 Compare July 27, 2017 23:42

aduth previously requested changes Jul 28, 2017

View reviewed changes

aduth reviewed Jul 29, 2017

View reviewed changes

westonruter added 3 commits July 28, 2017 19:58

Prevent serializing block attributes to comment which match defaults

d272cbd

Prevent Text block from getting false or empty className

35f81dd

Use createBlock to define default attributes

d6c8fd4

westonruter force-pushed the fix/omit-serializing-default-attributes branch from 71a07fc to d6c8fd4 Compare July 29, 2017 02:59

westonruter merged commit 03f8fe6 into master Jul 29, 2017

westonruter deleted the fix/omit-serializing-default-attributes branch July 29, 2017 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit serializing block attributes that match any default attribute values #1910

Omit serializing block attributes that match any default attribute values #1910

westonruter commented Jul 15, 2017 •

edited

Loading

westonruter commented Jul 25, 2017

westonruter commented Jul 27, 2017

aduth commented Jul 27, 2017

westonruter commented Jul 27, 2017

aduth left a comment

aduth Jul 27, 2017

westonruter Jul 27, 2017

westonruter Jul 27, 2017

westonruter Jul 28, 2017

aduth Jul 28, 2017

timmyc left a comment

timmyc Jul 27, 2017

westonruter Jul 27, 2017

westonruter commented Jul 27, 2017

codecov-io commented Jul 27, 2017 •

edited by codecov bot

Loading

aduth Jul 28, 2017

aduth Jul 28, 2017

westonruter Jul 28, 2017

aduth left a comment

westonruter commented Jul 29, 2017

Omit serializing block attributes that match any default attribute values #1910

Omit serializing block attributes that match any default attribute values #1910

Conversation

westonruter commented Jul 15, 2017 • edited Loading

westonruter commented Jul 25, 2017

westonruter commented Jul 27, 2017

aduth commented Jul 27, 2017

westonruter commented Jul 27, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jul 27, 2017

codecov-io commented Jul 27, 2017 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

westonruter commented Jul 29, 2017

westonruter commented Jul 15, 2017 •

edited

Loading

codecov-io commented Jul 27, 2017 •

edited by codecov bot

Loading