Lazy Images: Add react admin UI by ebinnion · Pull Request #8463 · Automattic/jetpack

ebinnion · 2018-01-04T21:08:38Z

When we launched lazy images in 5.7, we did not ship any react admin UI. This was partly because we wanted to do a soft launch and also because we would be releasing a new feature just before the holidays.

Now that we're past the holidays and have had some time to test the feature, let's get it out there!

Following the advice of @rickybanister, @michelleweber, and others in #8193, this PR creates a new section within the "Writing" tab called "Speed up your site". This PR, then moves the photon setting into this section and adds the lazy images setting as well. Further, this PR updates the descriptions for the Photon module to match what was suggested in #8193.

To test:

Checkout branch
Do a fresh yarn build to rebuild module translations
Go to "Writing" tab of Jetpack settings
Toggle both Photon and Lazy Images settings
Ensure that modules are properly enabled/disabled when they are toggled via UI

Screenshot:

ebinnion · 2018-01-04T22:07:19Z

Also, of note @rickybanister, I don't believe there is a "Tiled Galleries" setting anymore. Looking at the code, we simply turned on tiled galleries when photon was toggled on, and turned off tiled galleries when photon was turned off.

Check this line, where we're setting them both to the same thing.

In the issue referenced above, you mentioned this:

Note: I've removed the reference to This must be enable to use Jetpack's tiled gallery options. because we should mention that loading images via CDN is required from the tiled gallery setting instead.

So, we may need to add some wording so that users understand turning on Photon also turns on tiled galleries, or we need to add a separate tiled galleries toggle.

rickybanister · 2018-01-04T22:42:53Z

The work in this PR looks great, ship it.

Regarding tiled galleries, let's have @MichaelArestad chime in there. I really do want settings to not only control Jetpack, but help advertise it's features. Perhaps we can introduce something like this for tiled galleries (if the toggle is turned on for photon).

Something like this could work (borrowing from stats and other features that display a green checkbox when enabled):

eliorivero · 2018-01-04T22:50:08Z

modules/module-info.php

-		"Your images are automatically optimized for different display resolutions to serve the best
-		possible image quality. We also cache and serve them from our fast, global network (CDN)."
+		'Jetpack will optimize your images and serve them from the server location nearest
+		to your visitors. Using our global \'content delivery network\' will boost the loading speed of your site.'


I don't think we need the single quotes around content delivery network since we're not talking about that word, or being sarcastic about it, or making a reference of it inside a bigger context.
If the intention here is to highlight it, quotes are not highlight devices and italic or bold should be used instead.

I agree. Maybe instead we could just say "Using our global network..."

I'm impartial. I was copying how @rickybanister had laid out his copy in the referenced issue. Glad to change if we come to a consensus on what to change it to.

Agreed with @eliorivero on removing the quotes.

MichaelArestad · 2018-01-04T22:50:23Z

I think this looks good! Can we also add this to Calypso exactly the same way? I would change the last line of the explanation text to:

"Lazy loading" means the images visible on the page first will load before the rest.

This way folks don't make a presumption that only the visible images will load (and no others).

@rickybanister I think we could do something similar for Tiled galleries. Maybe the circle checkmark instead of the big one, but same idea. If Photon is disabled, it could have a simple button to enable photon or at least scroll the user down to the Photon setting (but of course don't say "Photon").

eliorivero · 2018-01-04T22:53:22Z

modules/module-info.php

+	esc_html_e(
+		'Normally, opening a web page causes it to load every image it contains, even those
+		that a reader hasn\'t yet scrolled down to see. "Lazy loading" means that only the images
+		actually visible on the screen will load.'


Maybe we don't need to explain lazy loading and instead what this toggle do?
I'd change
"Lazy loading" means that only the images actually visible on the screen will load.
to
Enable this to load the images visible to users as they scroll down

I would preserve the way @MichaelArestad has written it ("Lazy loading" means...) since it's a term not many Jetpack users will be familiar with.

Or we could opt for best of both worlds:

Enable this for "lazy loading" behavior, where only the images actually visible on the screen will load.

eliorivero · 2018-01-04T23:00:35Z

_inc/client/writing/speed-up-site.jsx

+import { ModuleToggle } from 'components/module-toggle';
+
+const SpeedUpSite = moduleSettingsForm(
+	React.createClass( {


We used React.createClass in the initial UI, but we should use ES6 classes for new components. For example

export const SSO = moduleSettingsForm( class extends Component {

in jetpack/_inc/client/security/sso.jsx

eliorivero · 2018-01-04T23:01:39Z

_inc/client/writing/speed-up-site.jsx

+			}
+
+			const photon = this.props.module( 'photon' ),
+				lazyImages = this.props.module( 'lazy-images' );


We should follow Calypso's guidelines of placing each const definition in a new line:

const photon = this.props.module( 'photon' ); const lazyImages = this.props.module( 'lazy-images' );

ebinnion · 2018-01-04T23:17:06Z

Can we also add this to Calypso exactly the same way? I would change the last line of the explanation text to:

To clarify on how we add this to Calypso:

For WordPress.com sites, we would add the "Speed up your site" section with only the lazy images toggle
For Jetpack sites, we would copy the layout and copy over exactly

cc @MichaelArestad

ebinnion · 2018-01-04T23:25:14Z

I just pushed up a commit to address @eliorivero's feedback about using an es6 class and putting constants on their own line.

MichaelArestad · 2018-01-04T23:44:04Z

For WordPress.com sites, we would add the "Speed up your site" section with only the lazy images
For Jetpack sites, we would copy the layout and copy over exactly

@ebinnion exactly. I think we could try making them almost identical later on with the checkmark idea @rickybanister mocked up for the settings that are just always on.

eliorivero · 2018-01-05T17:20:38Z

modules/lazy-images.php

 /**
 * Module Name: Lazy Images
- * Module Description: Improve performance by loading images just before they scroll into view
+ * Module Description: "Lazy load" images


IMHO the previous description was much more descriptive for non tech savvy users.

oskosk · 2018-01-05T17:39:41Z

I've just added a commit (a23b94b) factoring out decodeEntities into lib/decode-entities thinking of the children and the future...

rickybanister · 2018-01-05T18:03:06Z

Final_final_v4_last_copy

"Lazy-load" images
Improve your site's speed by only loading images visible on the screen. New images will load just before they scroll into view. This prevents viewers from having to download all the images on a page all at once, even ones they can't see.

rickybanister · 2018-01-05T18:20:05Z

Updated to add 'all at once' to the copy above

ebinnion · 2018-01-09T02:15:50Z

I've just pushed up another commit which should address everyone's suggestions about the copy. Here's the latest screenshot.

rickybanister · 2018-01-09T16:51:33Z

Beautiful @ebinnion. Thanks for accommodating all our copywriting!

oskosk

LGTM!

oskosk

@ebinnion , Derek noticed that you can't get a search result in the settings tab when typing "lazy". You do get results when writing "speed...". Do you think it should be on the results for that keyword ?

dereksmart

Looks great, one request: Let's add it to found modules so it is discoverable in search?

https://github.com/Automattic/jetpack/pull/8463/files#diff-2733a8e3221abee64d737c90e800d1a5R42

because yes

ebinnion · 2018-01-15T20:37:13Z

I've rebased against master and pushed up a change so that lazy images shows up in module search.

dereksmart

LGTM!

* Changelog 5.8: create base for changelog. * Update 5.8 release post link * fix 5.8 release date * Updates to plugin description * Changelog: add #8499 * Changelog: add #8506 * Changelog: add #8509 * Changelog: add #8516 * Changelog: add #8517 * Changelog: add #8523 * Changelog: add #8547 * Changelog: add #8496 * Changelog: add #8584 * Changelog: add #8595 * Changelog: add #8445 * Changelog: add #8431 * Changelog: add #8284 * Changelog: add #8270 * Changelog: add #8124 * Changelog: add #8581 * Changelog: add #8463 * Changelog: add #8568 (#8646) * Updates to testing list and changelog * Changelog: add #8443 * Changelog: add #8459 * Changelog: add #8469 * Changelog: add #8464 * Changelog: add #8478 and #8479 * Changelog: add #8483 * Changelog: add #8488 * Changelog: add #8513 * Changelog: add #8555 * Changelog: add #8565 * Changelog: add #8601 * Changelog: add #8612 * Changelog: add first pass at Search items. * Changelog: add more info to help test Search. * Changelog: add #8144 * Changelog: add #8313 * Changelog: add #8419 * Changelog: add #8465 * Changelog: add #8515 * Changelog: add #8587 * Changelog: add #8591 * Changelog: add #8659 * Changelog: add #8661 * Changelog: add #8671 * Changelog: add 5.7.1 to archived changelog too. * Reverted changes to readme, removed entry about backups.

ebinnion added [Feature] Lazy Images [Status] In Progress labels Jan 4, 2018

ebinnion added this to the 5.8 milestone Jan 4, 2018

ebinnion requested a review from a team as a code owner January 4, 2018 21:08

ebinnion force-pushed the update/lazy-images-add-ui branch from e8e7af8 to dd2decb Compare January 4, 2018 22:00

ebinnion added [Status] Needs Design Review Design has been added. Needs a review! and removed [Status] In Progress labels Jan 4, 2018

eliorivero reviewed Jan 4, 2018

View reviewed changes

eliorivero reviewed Jan 5, 2018

View reviewed changes

ebinnion self-assigned this Jan 9, 2018

ebinnion added the [Status] Needs Review This PR is ready for review. label Jan 9, 2018

oskosk approved these changes Jan 9, 2018

View reviewed changes

oskosk added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Design Review Complete and removed [Status] Needs Review This PR is ready for review. [Status] Needs Design Review Design has been added. Needs a review! labels Jan 9, 2018

oskosk previously requested changes Jan 10, 2018

View reviewed changes

dereksmart reviewed Jan 10, 2018

View reviewed changes

ebinnion and others added 7 commits January 15, 2018 14:15

Lazy Images: Add react admin UI

98e8559

Lazy Images: Add Lazy Images descriptions in UI

c382970

Lazy Images: Remove unnecessary unescape require

692c210

Lazy Images: Address JS style feedback for setting UI

a186957

Factor out decodeEntities into client/lib/decode-entities

d02e2ff

Admin Page: Update Lazy image and photon feature copy

cf6c25f

Lazy Images: Ensure lazy images shows up in module search

717d6e4

ebinnion force-pushed the update/lazy-images-add-ui branch from a38f4c2 to 717d6e4 Compare January 15, 2018 20:36

dereksmart approved these changes Jan 17, 2018

View reviewed changes

dereksmart merged commit 4862b3c into master Jan 17, 2018

matticbot added the [Status] Needs Changelog label Jan 17, 2018

dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 17, 2018

dereksmart deleted the update/lazy-images-add-ui branch January 17, 2018 15:21

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jan 25, 2018

jeherve added a commit that referenced this pull request Jan 25, 2018

Changelog: add #8463

52cc915

ebinnion mentioned this pull request Jan 26, 2018

Site Settings: Jetpack: Add lazy images setting Automattic/wp-calypso#21867

Closed

jeherve mentioned this pull request Jan 30, 2018

Module List: Lazy Images item reloads page instead of linking to support #8502

Closed

ebinnion mentioned this pull request Feb 1, 2018

Site Settings: Jetpack: Add lazy images setting for Jetpack sites Automattic/wp-calypso#22053

Merged

Conversation

ebinnion commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebinnion commented Jan 4, 2018

Uh oh!

rickybanister commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelArestad commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Jan 4, 2018

Uh oh!

ebinnion commented Jan 4, 2018

Uh oh!

MichaelArestad commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskosk commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickybanister commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickybanister commented Jan 5, 2018

Uh oh!

ebinnion commented Jan 9, 2018

Uh oh!

rickybanister commented Jan 9, 2018

Uh oh!

oskosk left a comment

Choose a reason for hiding this comment

Uh oh!

oskosk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dereksmart left a comment

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Jan 15, 2018

Uh oh!

dereksmart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ebinnion commented Jan 4, 2018 •

edited

Loading

oskosk commented Jan 5, 2018 •

edited

Loading

rickybanister commented Jan 5, 2018 •

edited

Loading

oskosk left a comment •

edited

Loading