Skip to content

Shortcodes: minify jQuery Cycle script#6024

Merged
dereksmart merged 3 commits intomasterfrom
clean/minify-jquery-cycle
Jan 3, 2017
Merged

Shortcodes: minify jQuery Cycle script#6024
dereksmart merged 3 commits intomasterfrom
clean/minify-jquery-cycle

Conversation

@eliorivero
Copy link
Copy Markdown
Contributor

@eliorivero eliorivero commented Dec 31, 2016

I noticed this on my new site elio.blog and searched for an issue here and there's this one that this PR now fixes #3335. I know that #5620 exists but I'm not sure if we need to minify these kind of files on each build since we won't be changing them often. So, here's this PR that specifically solves #3335.

Also, the reason why I noticed this script was because after running a test in the site I saw the script loaded, and I wasn't using it at all. I realized that when Infinite Scroll is supported, this script and others are always loaded. However, in my tests this wasn't true. I tested with this same branch and whenever the post that had a slideshow gallery was loaded, the script was loaded, so it's not necessary to always load it.

Changes proposed in this Pull Request:

  • use minified jQuery Cycle JS
  • don't load the script always when Infinite Scroll is activated

Testing instructions:

  • add a slideshow gallery and verify it works normally
  • test Infinite Scroll and add a slideshow in a post that is displayed after IS fetches more entries and verify it works

Proposed changelog entry for your changes:

Shortcodes: the jQuery Cycle script used by slideshow galleries is now minified resulting in faster loading times.

cc @lancewillett for review

@eliorivero eliorivero added [Feature] Shortcodes / Embeds [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Dec 31, 2016
@eliorivero eliorivero added this to the 4.5 milestone Dec 31, 2016
@eliorivero eliorivero self-assigned this Dec 31, 2016
Copy link
Copy Markdown
Contributor

@lancewillett lancewillett left a comment

Choose a reason for hiding this comment

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

I noticed the related test isn't active. Want to re-enable it?

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.

Do we need to bump the version number here to bust cache?

@lancewillett
Copy link
Copy Markdown
Contributor

Tested locally with Jetpack dev environment, works well. Going to give it a spin on WP.com test site also.

@lancewillett lancewillett added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 3, 2017
Copy link
Copy Markdown
Contributor

@lancewillett lancewillett left a comment

Choose a reason for hiding this comment

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

Should the file be renamed to jquery.cycle.min.js instead?

@lancewillett
Copy link
Copy Markdown
Contributor

lancewillett commented Jan 3, 2017

Tested successfully on WP.com. 👍 (I renamed the file there to add in min, though.)

Related WP.com code change (pending review): D3841-code

@eliorivero eliorivero force-pushed the clean/minify-jquery-cycle branch from a0d8020 to 99a6e3a Compare January 3, 2017 17:33
@eliorivero eliorivero force-pushed the clean/minify-jquery-cycle branch from 310bcb2 to 4775c44 Compare January 3, 2017 17:48
@eliorivero
Copy link
Copy Markdown
Contributor Author

@lancewillett I have:

  • renamed file to jquery.cycle.min
  • updated version to bust cache
  • re-enabled the tests and modify the slideshow.php file to pass tests

@eliorivero eliorivero removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 3, 2017
@lancewillett
Copy link
Copy Markdown
Contributor

@eliorivero Tests out both Jetpack local and on WP.com, looks great.

@lancewillett lancewillett added the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 3, 2017
@dereksmart
Copy link
Copy Markdown
Contributor

Nice, LGTM -- Thanks @lancewillett for testing on wpcom!

@dereksmart dereksmart merged commit 80d82ca into master Jan 3, 2017
@dereksmart
Copy link
Copy Markdown
Contributor

merged to 4.5 fb7e160

@dereksmart dereksmart deleted the clean/minify-jquery-cycle branch January 3, 2017 19:01
jeherve added a commit that referenced this pull request Jan 9, 2017
dereksmart pushed a commit that referenced this pull request Jan 17, 2017
CHangelog: add #5457

Changelog: add #5487

Changelog: add #5708

Changelog: add #5879

Changelog: add #5932

Changelog: add #5963

Changelog: add #5968

Changelog: add #5996

Changelog: add #5998

Changelog: add #5999

Changelog: add #6012

Changelog: add #6013

Changelog: add #6014

Changelog: add #6015

Changelog: add #6023

Changelog: add #6024

Changelog: add #6030

Changelog: add #5465

CHangelog: add #6063

Changelog: add #6025

Changelog: add #5974

Changelog: add #6059

Changelog: add #6046

Changelog: add #5418

Changelog: move things around and add missing information.

Changelog: add #5565

Changelog: add #6087

Changelog: add #6095
dereksmart pushed a commit that referenced this pull request Jan 17, 2017
Changelog: add #5867

Changelog: add #5874

Changelog: add #5905

Changelog: add #5906

Changelog: add #5931

Changelog: add #5933

Changelog: add #5934

Bring over 4.4.2 changelog from branch-4.4

@see 18012a3

Changelog: add #5976, #5978, #5983

Changelog: add #5917

Changelog: add #5832

Changelog: add 4.4.2 release post link.

CHangelog: add #5457

Changelog: add #5487

Changelog: add #5708

Changelog: add #5879

Changelog: add #5932

Changelog: add #5963

Changelog: add #5968

Changelog: add #5996

Changelog: add #5998

Changelog: add #5999

Changelog: add #6012

Changelog: add #6013

Changelog: add #6014

Changelog: add #6015

Changelog: add #6023

Changelog: add #6024

Changelog: add #6030

Changelog: add #5465

CHangelog: add #6063

Changelog: add #6025

Changelog: add #5974

Changelog: add #6059

Changelog: add #6046

Changelog: add #5418

Changelog: move things around and add missing information.

Changelog: add #5565

Changelog: add #6087

Changelog: add #6095

Readme: add @tyxla to the list of contributors.

Improved changelog for your readability and enjoyment

updated the release date

finalizing the changelog with a few more edits
zinigor pushed a commit that referenced this pull request Sep 8, 2017
…w minified resulting in faster loading times.

Summary: Syncs changes in Jetpack to WP.com: #6024

Test Plan:
1. Apply patch to your WP.com sandbox (`arc patch D3841`).
2. Add a slideshow gallery to a post and verify it works normally.
3. Test with Infinite Scroll on and add a slideshow in a post that is displayed after IS fetches more entries and verify that both slideshows still work.

Reviewers: jeherve, eliorivero

Reviewed By: eliorivero

Differential Revision: https://[private link]

Merges r148465-wpcom.
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use minified jquery.cycle.js in place of expanded version

6 participants