#31793 closed defect (bug) (fixed)
Theme Switcher: Lazy load theme screenshots
Reported by: | ocean90 | Owned by: | ocean90 |
---|---|---|---|
Milestone: | 4.2 | Priority: | high |
Severity: | normal | Version: | 4.2 |
Component: | Customize | Keywords: | needs-patch |
Focuses: | javascript | Cc: |
Description
Currently all theme screenshots are loaded, although the section is hidden at first. That needs to be improved.
Attachments (4)
Change History (24)
#2
@
10 years ago
- Keywords has-patch added; needs-patch removed
I'm attaching here a patch I also attached on #28580, and as I commented, in 31793.diff the ThemeControl
's renderContent
method gets deferred until the containing section is expanded, meaning that the screenshots won't load until the Theme Switcher panel is opened. Given the lateness of the release cycle, I think such a Theme Switcher-specific loading optimization should be what is in scope. /cc @mattwiebe
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
10 years ago
#5
@
10 years ago
31793.patch is a first pass for lazy loading screenshots. Screenshots are loaded as soon as the control comes into the view. Doesn't handle search queries yet.
@westonruter: Just noticed the missing props in [31944], sorry!
#6
follow-up:
↓ 8
@
10 years ago
I feel like we should probably load a bit earlier than that, so that there's time for the image to fully download on a normal/slower connection before the user sees the image. Maybe load 2-3 screenshots ahead?
#8
in reply to:
↑ 6
@
10 years ago
- Status changed from assigned to reviewing
Replying to celloexpressions:
I feel like we should probably load a bit earlier than that, so that there's time for the image to fully download on a normal/slower connection before the user sees the image. Maybe load 2-3 screenshots ahead?
@ocean90: 31793.2.diff now implements this. Props redemption ;-)
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#13
@
10 years ago
- Keywords needs-patch added; has-patch commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#14
follow-up:
↓ 17
@
10 years ago
31793.3.diff adds a isRendered
property to api.ThemeControl
. Not sure if it's the right approach.
#15
@
10 years ago
I'm not sure if 31793.3.diff is intended to help with the empty screenshots mentioned in #31794. I gave it a run through on an iPhone 6+ and still see the empty shots.
This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.
10 years ago
#17
in reply to:
↑ 14
@
10 years ago
Replying to ocean90:
31793.3.diff adds a
isRendered
property toapi.ThemeControl
. Not sure if it's the right approach.
This patch makes sense, as we don't want to re-render the content if it has already been rendered. However, as with @ryan I still am seeing the empty screenshot, but only starting with the 7th theme listed:
<div class="theme-screenshot"> <img data-src="http://src.wordpress-develop.dev/wp-content/themes/twentyten/screenshot.png" alt=""> </div>
New patch on #28580 seems to improve this, and also improve performance for other situations where people may be doing this type of thing.