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

Reinstate correct cover block attributes for deprecations #15449

Merged
merged 5 commits into from May 6, 2019

Conversation

@talldan
Copy link
Contributor

commented May 6, 2019

Description

Fixes #15441

The incorrect attributes were specified in the deprecated.js file for the cover block. It looks like the attributes for the button block were accidentally pasted into the file instead of the attributes for the cover block.

How has this been tested?

  1. Roll back to a past version of Gutenberg (I used 5.2)
  2. Created a new post with a cover block and save it
  3. Roll forwards to the latest version of Gutenberg
  4. Open up the post created in step 2.
  5. Observe that the cover block fails validation
  6. Checkout this branch and reload the post
  7. Observe that the block works again.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@gziolo
Copy link
Member

left a comment

I must have copied those attributes from the wrong file... 😅 I will test this PR as soon as I get to my desktop 👍

We will need to add integration test for this deprecated versions of block, similar to what I do in #15268.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Thanks @gziolo. I still have the block markup, so I can add an integration test. I'll just break for some lunch and then add one. 👍

@gziolo

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Thanks @gziolo. I still have the block markup, so I can add an integration test. I'll just break for some lunch and then add one. 👍

It would be awesome. When you have HTML markup, you just need to put it in the proper folder following the pattern used for other files. Then you run npm run fixtures:regenerate and 3 remaining files will get generated :)

@talldan talldan requested review from aduth, nerrad and ntwb as code owners May 6, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@gziolo I've added one for the latest deprecation, but there are a couple of older ones still. Is it worth going back through all the past deprecations for the cover block and adding those too? If so, would the idea be to add the markup to the same core__cover__deprecated file, or create new files (core__cover__deprecated_1, core__cover__deprecated_2)?

@gziolo

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@gziolo I've added one for the latest deprecation, but there are a couple of older ones still. Is it worth going back through all the past deprecations for the cover block and adding those too? If so, would the idea be to add the markup to the same core__cover__deprecated file, or create new files (core__cover__deprecated_1, core__cover__deprecated_2)?

In #15268 @ellatrix suggested to include new file per deprecation entry, so let's try to do it here as well. I think, you will have to use core__cover__deprecated-1.html. Well, the existing names aren't consistent but it is closer to how file names are handled in general.

@talldan talldan force-pushed the fix/cover-block-deprecated-attributes branch from a5b3f52 to 379efb0 May 6, 2019

@@ -0,0 +1,3 @@
<!-- wp:cover {"url":"https://cldup.com/uuUqE_dXzy.jpg","id":35} -->
<div class="wp-block-cover has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg)"><p class="wp-block-cover-text"><strong>Cover Block</strong></p></div>

This comment has been minimized.

Copy link
@gziolo

gziolo May 6, 2019

Member

Can you copy one of the existing images encoded with data:*. I think @jorgefilipecosta updated all urls to avoid network calls in tests or something like that.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@gziolo Wasn't straightforward getting markup for the older deprecations, just about managed it. 😄

I've added those fixtures the way we discussed. If another deprecation is added, all someone has to do is add a core__cover__deprecated-4.html file and regenerate.

🤞 the tests pass.

@@ -0,0 +1,3 @@
<!-- wp:cover {"url":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==","id":34,"className":"wp-block-cover-image"} -->

This comment has been minimized.

Copy link
@gziolo

gziolo May 6, 2019

Member

It doesn't look it gets converted properly.

This comment has been minimized.

Copy link
@talldan

talldan May 6, 2019

Author Contributor

Managed to figure this one out as well. Had to add:

		supports: {
			className: false,
		},

So that the change in class name from wp-block-cover-image to wp-block-cover works correctly.

Also added a migration function so that the inner text content is converted to a paragraph block.

@@ -0,0 +1,3 @@
<!-- wp:cover {"url":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==","id":34} -->
<div class="wp-block-cover has-background-dim" style="background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==)"><div class="wp-block-cover__inner-container"></div></div>

This comment has been minimized.

Copy link
@gziolo

gziolo May 6, 2019

Member

<strong>Cover Block</strong> is missing in the output.

This comment has been minimized.

Copy link
@talldan

talldan May 6, 2019

Author Contributor

Now resolved by adding a migration function.

@gziolo

gziolo approved these changes May 6, 2019

Copy link
Member

left a comment

Awesome work @talldan. Now, I think it's essential to keep all deprecations covered with integration tests to ensure they don't regress in the future. Thanks for fixing all bugs down the road :)

@gziolo gziolo merged commit d137b09 into master May 6, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the fix/cover-block-deprecated-attributes branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.