Skip to content

[Gutenberg] Image size improvements#12983

Merged
etoledom merged 4 commits intodevelopfrom
gutenberg/image-size-improvements
Nov 26, 2019
Merged

[Gutenberg] Image size improvements#12983
etoledom merged 4 commits intodevelopfrom
gutenberg/image-size-improvements

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

Fixes n1 from wordpress-mobile/gutenberg-mobile#1593

In this PR, we avoid using the PhotonAPI helper, which was aggressively stripping the image size properties from the URL.

The downloadImage method of RCTImageURLLoader is used by the JS function Image.getSize() to retrieve the original image and calculate its size.

I also took this opportunity to add a simple in-memory image cache to avoid fetching the same image multiple times.

To test:

  • On a WPCom site:
  • Add an image block.
  • Add an image from any source.
  • Change the image size to thumbnail.
  • Check that the size of the image visually changes.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@etoledom etoledom added the Gutenberg Editing and display of Gutenberg blocks. label Nov 20, 2019
@etoledom etoledom added this to the 13.8 milestone Nov 20, 2019
@etoledom etoledom self-assigned this Nov 20, 2019
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 20, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@mchowning
Copy link
Copy Markdown
Contributor

I also took this opportunity to add a simple in-memory image cache to avoid fetching the same image multiple times.

👍 I have caching turned on for Android as well.

requestURL = WPImageURLHelper.imageURLWithSize(size, forImageURL: requestURL)
request = URLRequest(url: requestURL)
} else {
} else if allowPhotonAPI {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@etoledom Can you clarify when allowPhotonAPI is set to true/false is there a related Gutenberg-Mobile PR where I can see the use of this argument?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The previous behavior was to always fall back to the Photon API. With this change and setting allowPhotonAPI = true as default, all other instances continue working in the same way.

This PR uses allowPhotonAPI = false to fetch the image from the original URL directly and avoid the Photon resizing that affects the RN Image.getSize method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm giving this a second look trying to avoid introducing allowPhotonAPI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So, playing with the image scale we are able to get the desired result using the Photon Helper.
I believe it's ready for another 👀 @SergioEstevao 🙏

@etoledom etoledom force-pushed the gutenberg/image-size-improvements branch from efdb2c4 to b14c227 Compare November 26, 2019 10:44
Copy link
Copy Markdown
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Working great, the new cache feature makes it so much better!

@etoledom etoledom merged commit efe6400 into develop Nov 26, 2019
@etoledom
Copy link
Copy Markdown
Contributor Author

Thank you!

@etoledom etoledom deleted the gutenberg/image-size-improvements branch November 26, 2019 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gutenberg Editing and display of Gutenberg blocks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants