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

Minify more JS #8329

Merged
merged 24 commits into from Dec 7, 2017
Merged

Minify more JS #8329

merged 24 commits into from Dec 7, 2017

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented Dec 7, 2017

Minifies javascript for more modules.

Running tally:

  • widgets
  • AtD
  • widget-visibility
  • custom-css
  • publicize
  • custom-post-types
  • sharedaddy
  • contact-form
  • photon
  • carousel
  • related-posts
  • tiled-gallery
  • likes
  • minileven
  • infinite-scroll
  • videopress
  • comment-likes
  • lazy-images

Results

While it's hard (and not always useful) to generalise regarding performance, on a typical page with a bunch of features enabled - contact form, carousel, related posts, videopress, etc - these changes can shave around 25% off the total javascript payload.

Most of these files achieved a compression ratio of about 50%.

To get even better results, we should look at reducing the size of hosted JS, for example removing jQuery from the Likes widget. This would shave another 100kb or so from every (uncached) page load.

@gravityrail gravityrail added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Dec 7, 2017
@gravityrail
Copy link
Contributor Author

This is ready to review

Copy link
Contributor

@ebinnion ebinnion left a comment

Choose a reason for hiding this comment

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

I pushed up a couple of fixes for non-minfied paths. Besides that, it tested well. LGTM.

class.photon.php Outdated
'jetpack-photon',
Jetpack::get_file_url_for_environment(
'_inc/build/photon/photon.min.js',
plugins_url( 'modules/photon/photon.js', JETPACK__PLUGIN_FILE )
Copy link
Contributor

Choose a reason for hiding this comment

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

When define( 'SCRIPT_DEBUG', true ); is set, I'm getting a 404 for this file. I believe this line should just be modules/photon/photon.js.

@gravityrail gravityrail merged commit 04fbdf4 into master Dec 7, 2017
@gravityrail gravityrail deleted the add/minify-more-js branch December 7, 2017 23:21
@gravityrail gravityrail removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Dec 7, 2017
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Dec 11, 2017
dereksmart pushed a commit that referenced this pull request May 2, 2018
Summary:
This merges this commit to the css file
 - adbde22

It also includes manual resolution for the divergent files:

- `mu-plugins/carousel/jetpack-carousel.php` -  significant commits:
   - #5469
   - 728c96f
   - 4b70301
   - 7fc7ebd
   - b4ad963
   - adbde22

- `mu-plugins/carousel/jetpack-carousel.js` - significant commits:
   - #8329
   - #7943
   - #8509

Test Plan:
Steps:

- Add an gallery widget to a site.
- Click on an image in the gallery
- The carousel should open and the navigation should work as expect.
- Check the dev console for any JS errors

Reviewers: dereksmart, zinigor

Reviewed By: zinigor

Subscribers: zinigor, brbrr, dereksmart

Differential Revision: https://[private link]

Merges r174217-wpcom.
dereksmart added a commit that referenced this pull request May 2, 2018
* Likes: update Carousel to remove Likes support, to match Jetpack.

Resolve divergences from Jetpack as well for related files.

Fixes #7133.

Merges r133276-wpcom.

* Carousel: better Jetpack JS hiding, see r133276.

Merges r133288-wpcom.

* Carousel: revert part of r133276 where galleries are auto closed.

Reported in Slack: https://a8c.slack.com/archives/triage/p1459368181000158

Jetpack related change: 36e529d

Merges r133533-wpcom.

* Jetpack: Sync carousel module changes

Summary: @michiecat mentioned today that a client was having an issue where opening a gallery, closing a gallery, and then clicking the back button was reopening the gallery. It looks like we have fixed this in Jetpack, but we need to sync those changes back to WPCOM so our WP.com and VIP users can get those changes.

Test Plan:
Check out patch. Sandbox site. Open gallery. Navigate through slides. Close gallery. Hit back button. Gallery should not open.

**Bonus feature**
Single images linked to the attachment URL should now open in a lightbox.  via #5469

Reviewers: samhotchkiss, dereksmart, eliorivero

Reviewed By: dereksmart, eliorivero

Differential Revision: https://[private link]

Merges r147170-wpcom.

* Carousel: merge changes from Jetpack

Summary:
This merges this commit to the css file
 - adbde22

It also includes manual resolution for the divergent files:

- `mu-plugins/carousel/jetpack-carousel.php` -  significant commits:
   - #5469
   - 728c96f
   - 4b70301
   - 7fc7ebd
   - b4ad963
   - adbde22

- `mu-plugins/carousel/jetpack-carousel.js` - significant commits:
   - #8329
   - #7943
   - #8509

Test Plan:
Steps:

- Add an gallery widget to a site.
- Click on an image in the gallery
- The carousel should open and the navigation should work as expect.
- Check the dev console for any JS errors

Reviewers: dereksmart, zinigor

Reviewed By: zinigor

Subscribers: zinigor, brbrr, dereksmart

Differential Revision: https://[private link]

Merges r174217-wpcom.

* Whitespace

* whitespace

* whitespace
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Apr 27, 2021
This was referenced Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants