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

Carousel optimizations #7943

Merged
merged 6 commits into from Oct 25, 2017
Merged

Carousel optimizations #7943

merged 6 commits into from Oct 25, 2017

Conversation

georgestephanis
Copy link
Member

Props @westi for the suggestion.

wip

George Stephanis added 2 commits October 5, 2017 13:50
Stop calling consecutive get_post()
a1fd26d 
and instead call one big `get_posts()` call for fewer db queries.

Props @westi for the suggestion.
Let's instead do one big str_replace with arrays instead of a bunch o…
1e8ebc3 
…f repeated ones.
@georgestephanis georgestephanis self-assigned this Oct 5, 2017
@georgestephanis georgestephanis requested a review from a team as a code owner October 5, 2017 18:03
@jeherve jeherve added [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] Performance [Type] Janitorial and removed [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Oct 6, 2017
George Stephanis added 2 commits October 6, 2017 15:07
We don't need numberposts.
a91fb59 
get_posts already sets that to the number of array items passed into include.
Add function to prime all the attachment meta in one go.
f137601
@georgestephanis georgestephanis added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 6, 2017
@georgestephanis
Copy link
Member Author

Comparing this branch to master with Query Monitor, I'm not actually seeing any decrease in the number of queries. Maybe there's some optimization already going on behind the scenes here that makes this redundant?

@georgestephanis
Copy link
Member Author

Even if this doesn't save any queries, we're at the least shifting to a single str_replace() call instead of doing it many times for each loop iteration. I'm in favor of the change, and conceptually having them all loaded and iterating through feels better than calling get_post() on each step.

Let's get this in.

oskosk
oskosk approved these changes Oct 25, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 25, 2017
@dereksmart dereksmart merged commit a11d9f4 into master Oct 25, 2017
@dereksmart dereksmart deleted the add/carousel-optimizations branch October 25, 2017 16:03
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 25, 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 and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 26, 2017
jeherve added a commit that referenced this pull request Oct 27, 2017
@jeherve
Changelog: add #7943
badb070
@jeherve jeherve mentioned this pull request Nov 3, 2017
Closed
@zinigor zinigor mentioned this pull request Nov 6, 2017
Merged
dereksmart pushed a commit that referenced this pull request May 2, 2018
@jhnstn @dereksmart
Carousel: merge changes from Jetpack
d9567d0 
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
@dereksmart
Resolve divergence in file: whitespace (#9469)
962df70 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskosk oskosk oskosk approved these changes

Assignees

@georgestephanis georgestephanis

Labels
[Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Focus] Performance [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@georgestephanis @oskosk @jeherve @dereksmart