Fix front-end performance of Tiled Gallery block #22547

guarani · 2022-01-28T13:55:43Z

#22445 reports a performance regression affecting page views where the Tiled Gallery is used.
This PR simply reverts the change to tiled-gallery/layout/mosaic/resize.js to its previous contents (prior to #21849) and creates a native-specific file, resize.native.js, containing the file's current contents.
I'm proposing leaving web-specific code in resize.native.js since removing it is lower priority and it wasn't immediately obvious which code to remove.

Fixes #22445

Changes proposed in this Pull Request:

Revert Tiled Gallery's resize.js file to a previous version to fix a performance regression on pages using the Tiled Gallery.

Jetpack product discussion

The only discussion is that on the issue this PR fixes.

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

No, it does not.

Testing instructions:

TO-DO

#22445 reports a performance regression affecting page views where the Tiled Gallery is used. This PR simply reverts the change to `tiled-gallery/layout/mosaic/resize.js` and creates a native-specific file, `resize.native.js`, containing the previous implementation. Later work could remove web-specific code from `resize.native.js`, but that is lower priority, so is not addressed here.

matticbot · 2022-01-28T13:55:49Z

Caution: This PR has changes that must be merged to WordPress.com
Hello guarani! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D73985-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2022-01-28T13:56:46Z

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.

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: March 1, 2022.
Scheduled code freeze: February 22, 2022.

guarani · 2022-01-28T14:01:20Z

I'm not sure how to answer the questions in the PR template:

Does this need to be tested by beta testers?
Which Milestone should this be added to? I'm not sure which to choose, beta/3.1.0, jetpack/10.6, etc

sgomes · 2022-01-31T11:41:16Z

Thank you, @guarani, this looks like a good approach! 👍 This should isolate @wordpress/element to the React Native build and keep it away from the web one.

That said, whether this works or not will depend on how the Jetpack build is set up, and how it treats these .native.js files. I'm not familiar with that, so I'll let someone from @Automattic/jetpack-crew take a look and let us know whether this will have the desired effect; that is, removing the dependency on @wordpress/element (aka wp-element) from the web build.

sgomes · 2022-01-31T11:42:27Z

Also, CC @jeherve, who was kind enough to file the original issue (#22445).

guarani · 2022-01-31T14:23:51Z

Thanks @sgomes!

I've moved this from Draft to Ready for Review 🙇

guarani · 2022-01-31T14:29:29Z

I've merged the latest from master into this branch because I see some failed test such as:

Build / Build all projects (pull_request): pnpm run build-production-extensions exited with code 1
Jetpack Boost E2E Tests / Check if Boost E2E tests should run (pull_request: Failing at "Run pnpm install"

I hope that fixes the failing CI checks.

anomiex · 2022-01-31T15:33:26Z

The build is failing with this error:

  [2:24.757] [build-production-extensions] ERROR in ./extensions/blocks/tiled-gallery/layout/mosaic/index.js 118:20-35
  [2:24.757] [build-production-extensions] export 'getColumnWidths' (imported as 'getColumnWidths') was not found in './resize' (possible exports: getGalleryRows, handleRowResize)
  [2:24.757] [build-production-extensions]  @ ./extensions/blocks/tiled-gallery/layout/index.js 16:0-30 90:69-75
  [2:24.757] [build-production-extensions]  @ ./extensions/blocks/tiled-gallery/edit.js 19:0-30 307:34-40
  [2:24.757] [build-production-extensions]  @ ./extensions/blocks/tiled-gallery/index.js 16:0-26 329:2-6
  [2:24.757] [build-production-extensions]  @ ./extensions/blocks/tiled-gallery/editor.js 5:0-35 6:21-25 6:27-35

This appears to be due to the fact that this PR is deleting the getColumnWidths function.

By migrating this function from the native to the web file, we prevent the build error described here on the web: #22547 (comment)

SiobhyB · 2022-02-01T18:59:26Z

@anomiex, thank you for highlighting that!

We've worked around this by moving the getColumnWidths to mosaic/index.js (the place it's called) in b66df7b. That way, the web isn't attempting to import it (which fails, as it's only defined in resize.native.js).

Looking forward to hearing whether this approach seems suitable as well as any other feedback on this PR. 🙇‍♀️

sgomes · 2022-02-07T14:24:34Z

Hey folks! Any progress on this one? It would be great to get a fix shipped on wpcom whenever possible 🙏

SiobhyB · 2022-02-07T14:32:20Z

@sgomes, thanks for checking in! It's been marked as ready for review on our side, so looking forward to hearing any feedback on the code or whether this is good to go :)

sgomes

Thank you, @SiobhyB! While I'm not an expert here, I can confirm that this does build correctly, and that it generates a view.assets.php without a dependency on wp-element 👍

I think it's only the Crew review missing, at this point.

…rontend-perf

github-actions · 2022-02-07T15:44:13Z

Great news! One last step: head over to your WordPress.com diff, D73985-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

jeherve · 2022-02-07T15:53:24Z

r239679-wpcom

SiobhyB · 2022-02-07T17:24:44Z

Thank you @sgomes and @jeherve! 🙇‍♀️

guarani · 2022-02-07T18:49:00Z

Thank you @sgomes and @jeherve! 🙇‍♀️

💯 Thanks a lot, @sgomes, @jeherve and @SiobhyB!

guarani added [Feature] Tiled Gallery A different way to display image galleries on your site, in different organizations and shapes. [Block] Tiled Gallery labels Jan 28, 2022

matticbot added Touches WP.com Files labels Jan 28, 2022

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jan 28, 2022

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 28, 2022

guarani added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 28, 2022

guarani mentioned this pull request Jan 28, 2022

Tiled Galllery Block: update Platform implementation to avoid pulling too many dependencies #22445

Closed

sgomes requested a review from a team January 31, 2022 11:41

Merge branch 'master' into fix/tiled-gallery-frontend-perf

3c6dcfc

guarani marked this pull request as ready for review January 31, 2022 14:30

guarani marked this pull request as draft January 31, 2022 21:01

Siobhan added 2 commits February 1, 2022 18:04

Migrate getColumnWidths() to web file

b66df7b

By migrating this function from the native to the web file, we prevent the build error described here on the web: #22547 (comment)

Merge branch 'master' into fix/tiled-gallery-frontend-perf

a2efdc9

SiobhyB marked this pull request as ready for review February 1, 2022 18:59

SiobhyB requested review from a team and removed request for a team February 1, 2022 18:59

jeherve added [Status] Needs Team Review and removed [Status] In Progress labels Feb 2, 2022

jeherve added this to the jetpack/10.6.1 milestone Feb 2, 2022

sgomes approved these changes Feb 7, 2022

View reviewed changes

sgomes added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Feb 7, 2022

Merge remote-tracking branch 'origin/master' into fix/tiled-gallery-f…

34bf21f

…rontend-perf

jeherve assigned guarani Feb 7, 2022

jeherve approved these changes Feb 7, 2022

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 7, 2022

jeherve merged commit 2bc858a into master Feb 7, 2022

jeherve deleted the fix/tiled-gallery-frontend-perf branch February 7, 2022 15:43

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 7, 2022

jeherve modified the milestones: jetpack/10.6.2, jetpack/10.7 Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix front-end performance of Tiled Gallery block #22547

Fix front-end performance of Tiled Gallery block #22547

guarani commented Jan 28, 2022 •

edited

Loading

matticbot commented Jan 28, 2022

github-actions bot commented Jan 28, 2022 •

edited

Loading

guarani commented Jan 28, 2022

sgomes commented Jan 31, 2022

sgomes commented Jan 31, 2022

guarani commented Jan 31, 2022

guarani commented Jan 31, 2022 •

edited

Loading

anomiex commented Jan 31, 2022

SiobhyB commented Feb 1, 2022 •

edited

Loading

sgomes commented Feb 7, 2022

SiobhyB commented Feb 7, 2022

sgomes left a comment •

edited

Loading

github-actions bot commented Feb 7, 2022

jeherve commented Feb 7, 2022

SiobhyB commented Feb 7, 2022

guarani commented Feb 7, 2022

Fix front-end performance of Tiled Gallery block #22547

Fix front-end performance of Tiled Gallery block #22547

Conversation

guarani commented Jan 28, 2022 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

matticbot commented Jan 28, 2022

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

guarani commented Jan 28, 2022

sgomes commented Jan 31, 2022

sgomes commented Jan 31, 2022

guarani commented Jan 31, 2022

guarani commented Jan 31, 2022 • edited Loading

anomiex commented Jan 31, 2022

SiobhyB commented Feb 1, 2022 • edited Loading

sgomes commented Feb 7, 2022

SiobhyB commented Feb 7, 2022

sgomes left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2022

jeherve commented Feb 7, 2022

SiobhyB commented Feb 7, 2022

guarani commented Feb 7, 2022

guarani commented Jan 28, 2022 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

guarani commented Jan 31, 2022 •

edited

Loading

SiobhyB commented Feb 1, 2022 •

edited

Loading

sgomes left a comment •

edited

Loading