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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Fix pixelated Tiled Gallery images #22230

Merged
merged 1 commit into from
Jan 7, 2022
Merged

[RNMobile] Fix pixelated Tiled Gallery images #22230

merged 1 commit into from
Jan 7, 2022

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Jan 5, 2022 •
edited
Loading

Fixes wordpress-mobile/gutenberg-mobile#4306

Related:

Changes proposed in this Pull Request:

This PR addresses the pixelated images we see on Tiled Galleries. The cause was a hard-coded image dimension set in the block's save function.

A workaround is to omit the width and height altogether, which allows images to be rendered at full-resolution on the frontend.

A solution is out of the scope of this PR and this is why: Image blocks, which we used in the Tiled Gallery here, don't expose their width or height via the image's block attributes. I can't find a way to ask an inner image block for its width or height. WordPress/gutenberg#23244 tracks the need to make the image block provide a default width and height, albeit for different reasons. WordPress/gutenberg#35982 explains the issue I'm running into, but I suspect this is a duplicate of the former issue.

For now, the above mentioned workaround seems to bring benefits to the Tiled Gallery and doesn't have any downsides I'm aware of.

Screenshots

Images were pixelated Images are now full-resolution
Screen Shot 2022-01-05 at 18 51 08 Screen Shot 2022-01-05 at 18 55 17

Jetpack product discussion

wordpress-mobile/gutenberg-mobile#4306

Does this pull request change what data or activity we track or use?

No, it does not.

Testing instructions:

Setup to test from the main apps

  1. Run the packager from the add/tiled-gallery-block branch: npm run start:reset
  2. Run either of the main apps (WPiOS or WPAndroid)
  3. Open a new post in the main apps and expect the editor to load from the running packager
  1. Add a Tiled Gallery
  2. Add a couple of images
  3. Switch to the Square layout
  4. Preview the post
  5. Notice the images appear at their full-resolution

@github-actions github-actions bot added [Block] Tiled Gallery [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Jan 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • 🔴 Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • 🔴 Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

🔴 Action required: Please add missing changelog entries for the following projects: projects/plugins/jetpack

Use the Jetpack CLI tool to generate changelog entries by running the following command: jetpack changelog add.
Guidelines: /docs/writing-a-good-changelog-entry.md

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: January 11, 2022.
  • Scheduled code freeze: January 3, 2022.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 5, 2022
@guarani guarani mentioned this pull request Jan 5, 2022
Merged
2 tasks
@guarani guarani force-pushed the rnmobile/fix/tiled-gallery-block-pixelation branch from 4f46309 to a199961 Compare January 5, 2022 21:23
@guarani guarani changed the title Fix pixelated Tiled Gallery images [RNMobile] Fix pixelated Tiled Gallery images Jan 5, 2022
@guarani guarani requested a review from SiobhyB January 6, 2022 18:02
@guarani guarani marked this pull request as ready for review January 6, 2022 18:02
@guarani
Copy link
Contributor Author

guarani commented Jan 6, 2022 •
edited
Loading

Summary of failing tests:

JS tests

TypeError: row.querySelectorAll is not a function

I need to investigate this before submitting for review.

Code coverage

TypeError: row.querySelectorAll is not a function

I need to investigate this before submitting for review.

Changelogger use

This is expected to fail because we're merging to a feature branch and decided not to provide changelogs until we merge to master.

E2E post-connection tests

Re-running (might be flaky).

@guarani guarani removed the request for review from SiobhyB January 6, 2022 18:10
@guarani
Copy link
Contributor Author

guarani commented Jan 6, 2022

@SiobhyB sorry for the ping, will re-request a review once I've resolved the test failures.

@guarani
Copy link
Contributor Author

guarani commented Jan 6, 2022

Once #22252 is merged, I'll update this PR and it should resolve the above mentioned test failures.

@guarani
[not verified] Fix pixelated Tiled Gallery images
32bd281 
Removes a hard-coded dimension in the Tiled Gallery save function that was put there while iterating on the development of the block.
@guarani guarani force-pushed the rnmobile/fix/tiled-gallery-block-pixelation branch from f343ec1 to 32bd281 Compare January 7, 2022 13:48
@guarani guarani mentioned this pull request Jan 7, 2022
Closed
@guarani
Copy link
Contributor Author

guarani commented Jan 7, 2022 •
edited
Loading

I've fixed the failing CI checks (except the ones that are known failures) and made an issue for addressing the remaining points that this PR doesn't fix on the linked issue.

This is now ready for review, @SiobhyB 🙇

@guarani guarani requested a review from SiobhyB January 7, 2022 14:27
SiobhyB
SiobhyB approved these changes Jan 7, 2022
View reviewed changes
Copy link

@SiobhyB SiobhyB left a comment

Choose a reason for hiding this comment

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

LGTM! I confirmed the removal of the hard coded values fixes the pixelation and wasn't able to find any issues.

@guarani guarani merged commit bda38de into rnmobile/add/tiled-gallery-block Jan 7, 2022
@guarani guarani deleted the rnmobile/fix/tiled-gallery-block-pixelation branch January 7, 2022 16:31
@github-actions github-actions bot removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SiobhyB SiobhyB SiobhyB approved these changes

Assignees
No one assigned
Labels
[Block] Tiled Gallery [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guarani @SiobhyB @matticbot