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: Align Hook: Impossible to set no alignment when a default align exists #9634

Merged
merged 9 commits into from
Oct 8, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Sep 5, 2018

When we have a default value for an attribute, during the load of a document/block if that attribute is undefined the attribute gets the default value.
If we had a block that supports align but contained a default align we had a problem. If no align was set the post correctly saved the markup of no alignment set but the alignment attribute was not saved watching by the value of the attribute it was impossible to differentiate the state of default alignment being used, and the state of no alignment is used. The alignment attribute was just not stored in both cases.
If alignment was set to none (undefined), When the block was loaded no attribute was set, so the default value of alignment was used, but the block had the markup of alignment none, so the block becomes invalid.

This PR solves this problem by setting alignment to none when setting no alignment on the align hook, this way none is saved on the attribute and Gutenberg correctly differentiates the state of the block has the default align and the state the block has no alignment.

How has this been tested?

I used the following test block: https://gist.github.com/jorgefilipecosta/b733f1745531dc9758c6eba0838ae3f5; I checked on the code editor that if setting no alignment for this block we set alignment comment attribute to 'none' and no align class is added. And I checked that after removing the alignment saving the post and loading it again everything works as expected.

Screenshots

Before:
sep-05-2018 16-28-13

After:
sep-05-2018 15-46-36

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Sep 5, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Sep 5, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from 4231862 to 91b5ab6 Compare September 5, 2018 18:49
jorgefilipecosta added a commit that referenced this pull request Sep 5, 2018
const blockType = getBlockType( props.name );
const blockDefaultAlign = get( blockType, [ 'attributes', 'align', 'default' ] );
if ( blockDefaultAlign ) {
props.setAttributes( { align: 'none' } );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it set align to undefined? That seems to be the default behavior in blocks. If you set no alignment, no alignment class is applied to the block. (The Archives block is an exception to this rule as it applies an alignnone class, but I fixed that in #9557.)

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe setting it to none makes sense, but when applying the alignment classes, no class should be added if the value is none.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @ZebulanStanphill, when applying the classes we are not setting any class for this alignment and no alignnone should be set.
We can not set the align to undefined if a default alignment exists because default attributes are not saved. So if we set the alignment to undefined (as happened before this PR) for both the default alignment and no alignment the attribute was unset and it would not be possible to differentiate both states.
So in order to differentiate no alignment from default alignment, we add 'none' when alignment is not set. But nothing in the markup changes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Can you set null instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried but unfortunately, null values are also not persisted in the attribute comments making them behave exactly like undefined.

@aduth
Copy link
Member

aduth commented Sep 5, 2018

Will this invalidate existing blocks?

How does one unset an alignment if they really want to revert back to the default?

@ZebulanStanphill
Copy link
Member

@aduth You can unset alignment by clicking the already-enabled current alignment. Simple as that.

@jorgefilipecosta
Copy link
Member Author

Will this invalidate existing blocks?

The existing blocks that this PR affects are blocks with a default align and whose alignment was removed, these blocks are already invalid without this PR that's the bug we are solving. So we are not going to invalid more blocks with this PR.

How does one unset an alignment if they really want to revert back to the default?

Exactly like what @ZebulanStanphill referred. For example, in the columns block after we enable wide or full alignment the way to remove alignment is pressing the currently enabled alignment.

jorgefilipecosta added a commit that referenced this pull request Sep 7, 2018
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
jorgefilipecosta added a commit that referenced this pull request Sep 10, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
jorgefilipecosta added a commit that referenced this pull request Sep 10, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
jorgefilipecosta added a commit that referenced this pull request Sep 14, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
@jorgefilipecosta jorgefilipecosta requested a review from a team September 14, 2018 15:12
@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Sep 14, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch 2 times, most recently from 3671310 to d36d40a Compare September 21, 2018 23:29
@jorgefilipecosta
Copy link
Member Author

A set of end 2 end tests to the align hook were added in #10105. These tests are able to catch the bug being fixed here.

jorgefilipecosta added a commit that referenced this pull request Sep 26, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
jorgefilipecosta added a commit that referenced this pull request Sep 28, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
jorgefilipecosta added a commit that referenced this pull request Oct 3, 2018
Squashed commits:
[3dbd4cee8] Squashed commits:
[c9daa7092] Improved toolbar order (+6 squashed commits)
Squashed commits:
[f202072bf] #9634 cherry picked tempararly; This commit should be deleted as soon as the other PR is merged
[a09172386] Only use align wide & full
[1bf29ba] Corrected media labels
[173a298a1] Revisions to align style; Show resizer only when media is set.
[6a94b169d] Change sizing mechanism to apply width directly to the media elements (video/img)
[12ede9625] Half image Block
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from d36d40a to abba1b2 Compare October 3, 2018 19:26
@jorgefilipecosta
Copy link
Member Author

This PR was rebased, I think it is ready for a review. It is a blocker for #9416.

@aduth
Copy link
Member

aduth commented Oct 4, 2018

If this includes a fix, I might also expect there should be some test?

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.

Correct me if I'm wrong, but does this introduce a second "empty" value for default, so that now both undefined and 'none' mean "no alignment", the latter used in cases where an undefined would cause the default to take place? I guess I see this could be okay, in a same way undefined and null are different values in JavaScript, differentiated on explicitness (which makes me also wonder if null is a better value to avoid 'none' as a magic constant). To my last point, it still appears then that there is no way for a user to reset to the undefined value for a block which assigns a default value (also probably okay). It's also a bit confusing that regardless if a block specifies only a few alignments supported, this magic 'none' value will be added regardless.

const blockType = getBlockType( props.name );
const blockDefaultAlign = get( blockType, [ 'attributes', 'align', 'default' ] );
if ( blockDefaultAlign ) {
props.setAttributes( { align: 'none' } );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We could simplify this to assign nextAlign = 'none'; and it would fall through to same effect.

@aduth
Copy link
Member

aduth commented Oct 4, 2018

On the point of the magic constant, this would have the effect of adding and 'alignnone' class. I don't think we need to add this (if we store value as null), though given its existence in WordPress history, it is a thing which has some precedent. I'm not sure what purpose it serves, and personally I'd rather it not be there, fixing the original issue by storing the value as null.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch 2 times, most recently from 795b209 to abf5cc0 Compare October 4, 2018 16:12
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from ac27dd0 to 51a5068 Compare October 5, 2018 21:49
@mcsf mcsf modified the milestones: 4.0, 4.1 Oct 6, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from 51a5068 to ccb5cda Compare October 8, 2018 12:58
@jorgefilipecosta
Copy link
Member Author

This PR was rebased, everything was addressed it should be ready to merge :)

@gziolo
Copy link
Member

gziolo commented Oct 8, 2018

There is still one issue popping up:

screen shot 2018-10-08 at 16 28 54

Steps to reproduce:

  1. Insert the test block shared in the description.
  2. Unset alignment (null).
  3. Save the post.
  4. Refresh the page.
  5. You will see this warning popup.

@gziolo
Copy link
Member

gziolo commented Oct 8, 2018

It saves properly:

<!-- wp:test/no-align-default-align {"align":null} -->
<div style="outline:1px solid gray;padding:5px" class="wp-block-test-no-align-default-align">Test No Align and Default Align Block</div>
<!-- /wp:test/no-align-default-align -->

When I reload the page {"align":null} is no longer there, cc @mcsf @dmsnell - is that expected?


const getAlignmentToolbarLabels = async () => {
const buttonLabels = await page.evaluate( () => {
return Array.from(
Copy link
Member

Choose a reason for hiding this comment

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

This is cool in combination with map 👍

};

const verifyMarkupIsValid = async ( htmlMarkup ) => {
await switchToEditor( 'Code' );
Copy link
Member

Choose a reason for hiding this comment

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

Given that you set post content using data layer, do you still need to switch to Code Editor?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea of this switch was to make sure everything is unmounted and mounted again.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it was removed as it makes our tests faster and it does not seem required for this case.

@mcsf
Copy link
Contributor

mcsf commented Oct 8, 2018

When I reload the page {"align":null} is no longer there — is that expected?

As discussed, there should be no surprises at the level of the block parser:

wp.blockSerializationDefaultParser.parse(
  `<!-- wp:test/no-align-default-align {"align":null} -->
  <div style="outline:1px solid gray;padding:5px" class="wp-block-test-no-align-default-align">Test No Align and Default Align Block</div>
  <!-- /wp:test/no-align-default-align -->`
)[ 0 ].attrs
// { align: null }

The discrepancy you see should be happening at the block API level (@wordpress/blocks//api).

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from 484fd0c to d34c086 Compare October 8, 2018 15:19
@jorgefilipecosta
Copy link
Member Author

The PR was updated with some docs added. I updated the PR with to use an empty string for no alignment as suggested by @gziolo this avoids the need for blocks to specify the null type making #10338 unnecessary for this case. I still think the changes made in #10338 are important and support for multiple type attributes makes us closer to JSON schema and may be useful for other cases.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from 83d3e3c to dc2d739 Compare October 8, 2018 15:40
@jorgefilipecosta jorgefilipecosta force-pushed the fix/align-hook-default-align-no-align-bug branch from dc2d739 to e5b9af8 Compare October 8, 2018 15:43
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It works great after the latest changes applied. Thanks for working on that. With the set of e2e tests introduced it should be much easier to ensure this doesn't regress in the future. Great work 💯

@gziolo gziolo modified the milestones: 4.1, 4.0 Oct 8, 2018
@gziolo
Copy link
Member

gziolo commented Oct 8, 2018

We can land in 4.0 and unblock one of the PRs which depends on it.

@jorgefilipecosta jorgefilipecosta merged commit 9d419fa into master Oct 8, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/align-hook-default-align-no-align-bug branch October 8, 2018 16:41
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.

5 participants