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 gallery to image transform and inconsistent types used in gallery block #20084

Merged
merged 3 commits into from Feb 13, 2020

Conversation

@talldan
Copy link
Contributor

talldan commented Feb 7, 2020

Description

Fixes #20041

Fixes gallery to image transforms that were broken because the gallery block inconsistently stores image ids sometimes as numbers, but other times as strings. They're set as numbers initially when a gallery is created, but then as strings when loading a post and an existing gallery block is parsed.

The image block expects them to be numbers, so transforming an existing gallery block to images caused invalid image blocks.

This PR tidies up the inconsistencies:

  • Set type on gallery block attributes so that types are enforced.
  • Ensure attributes.images[ index ].id is always stored as a string by parsing the initial value.
  • Ensure the attributes.ids array is always updated as numbers by explicitly parsing the values.
  • Use attributes.ids when transforming galleries to images, as the attribute types are both numbers.
  • When looking up captions, always use the string value.
  • Always pass a number to the getMedia selector. It seems like if you try getMedia( 71 ) and getMedia( '71' ) the cache lookup doesn't resolve both ids as the same, resulting in a new HTTP request being made. This might be something to fix in a separate PR—entities could either (a) accept strings or numbers, but consistently convert them to the same type or (b) throw an error if the wrong type is used.

How has this been tested?

  1. Add a gallery with some images
  2. Save the post and reload
  3. Transform the gallery to images.
  4. Save and reload
  5. Expect that none of the image blocks should be invalid.

Types of changes

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

My feeling is this is non-breaking and doesn't require a block deprecation. Any existing galleries saved into posts will have their ids stored as strings in the data-id attribute. Every deprecation also stores ids this way. So setting the id type as string shouldn't cause an issue for existing blocks.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@pinarol

This comment has been minimized.

Copy link
Contributor

pinarol commented Feb 7, 2020

hi @talldan 👋 @mkevins and I are actually working on gutenberg-mobile project so you might want to invite someone comfortable reviewing web side as well.

Having said that, we definitely need to test this on mobile and see if it is breaking anything. @mkevins will help you if any change is needed on *.native.js files. My only request is not merging this before mobile tests are done. We'll check this shortly. Thanks!

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Feb 10, 2020

Thanks @pinarol, some validation/testing that this still works on mobile would be greatly appreciated.

I've pinged a few extra people for review.

@mkevins

This comment has been minimized.

Copy link
Contributor

mkevins commented Feb 12, 2020

Hi @talldan 👋 for some reason I got a 404 on the linked issue. Anyway, thanks for addressing the types for ids. I have tested this on WordPress-Android (merged locally onto #20043) with the following flows, and I haven't observed any regressions 🎉 :

  • Gallery block - Add caption to gallery images - steps
  • Gallery block - Choose from device (stay in editor) - Successful upload - steps
  • Gallery block - Choose from device (stay in editor) - Failed upload - steps
  • Gallery block - Choose from device (leave the editor) - Successful upload - steps
  • Gallery block - Choose from device (close and re-open the editor with ongoing uploads) - steps
  • Gallery block - Take a photo - steps
  • Gallery block - Choose from the free photo library - steps

This looks good to me, and I appreciate the comments 👍 .

Copy link
Contributor

draganescu left a comment

The transforms all work and the code looks perfect!

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Feb 13, 2020

Thanks for the reviews and testing.

@mkevins yep, for some reason the issue, #20041, disappeared, possibly deleted? I still have some email notifications about it, so that's how I know I'm not going crazy.

@talldan talldan merged commit 526b7d7 into master Feb 13, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@talldan talldan deleted the fix/gallery-to-image-transform branch Feb 13, 2020
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.