Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#31793 closed defect (bug) (fixed)

Theme Switcher: Lazy load theme screenshots

Reported by: ocean90's profile ocean90 Owned by: ocean90's profile 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)

31793.diff (894 bytes) - added by westonruter 10 years ago.
31793.patch (4.9 KB) - added by ocean90 10 years ago.
31793.2.diff (586 bytes) - added by westonruter 10 years ago.
https://github.com/xwp/wordpress-develop/pull/83
31793.3.diff (1.1 KB) - added by ocean90 10 years ago.

Download all attachments as: .zip

Change History (24)

#1 @celloexpressions
10 years ago

New patch on #28580 seems to improve this, and also improve performance for other situations where people may be doing this type of thing.

@westonruter
10 years ago

#2 @westonruter
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

#4 @ocean90
10 years ago

In 31944:

Customizer: Defer rendering theme controls until the section is displayed.

see #31793.

@ocean90
10 years ago

#5 @ocean90
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: @celloexpressions
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?

#7 @DrewAPicture
10 years ago

  • Owner set to ocean90
  • Status changed from new to assigned

#8 in reply to: ↑ 6 @westonruter
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

#10 @DrewAPicture
10 years ago

  • Keywords commit added

Tested this earlier, looked good to me.

This ticket was mentioned in Slack in #core by drew. View the logs.


10 years ago

#12 @ocean90
10 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 32088:

Customizer Theme Switcher: Lazy load theme screenshots.

props westonruter, ocean90.
fixes #31793.

#13 @ocean90
10 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@westonruter Since [31944] the controls are rendered each time the section expands. I don't see a reason for this, it should only run once.
Currently it breaks the behavior of [32088] where screenshots are missing on another expand.

@ocean90
10 years ago

#14 follow-up: @ocean90
10 years ago

31793.3.diff adds a isRendered property to api.ThemeControl. Not sure if it's the right approach.

#15 @ryan
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 @westonruter
10 years ago

Replying to ocean90:

31793.3.diff adds a isRendered property to api.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>

#18 @ocean90
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 32119:

Customizer Theme Switcher: Don't re-render a theme control if it has already been rendered.

Move initialization of $customizeSidebar to api.ThemesSection.initialize() to prevent cases where the result can be undefined.

props westonruter, ocean90.
fixes #31793.

#19 @azaozz
10 years ago

@ocean90, looking at [32088], maybe better to use:

source = $screenshot.attr( 'data-src' );

instead of

source = $screenshot.data( 'src' );

That will get the actual attribute from the element instead of relying on the jQuery caching.

#20 @ryan
10 years ago

Verifying that I no longer see empty screenshots on iPhone 5, Nexus 5, or iPhone 6+.

Note: See TracTickets for help on using tickets.