Encourage lighter images and videos #2509

kienstra · 2019-06-05T03:41:44Z

As Weston suggested, this replaces the 'full' image size with a custom size of 99999px (unlimited) by 1440px

There's more work needed, including applying the rest of the points in Guide users to not use heavy assets when creating stories #2332

Do this on the componentDidUpdate method, as it seems that this is where it is updated. Also, revert a change to webpack.config.js that I accidentally staged before.

This is only used on one place, so it is probably better in that class.

westonruter · 2019-06-05T04:23:41Z

Make sure this new image size is used for background image.

kienstra · 2019-06-05T04:41:17Z

Sure, I'll work on that now.

Use the newly added 'amp_story_page' image size for the AMP Story background image.

assets/src/stories-editor/blocks/amp-story-page/edit.js

kienstra · 2019-06-05T05:22:09Z

assets/src/stories-editor/components/higher-order/with-replaced-full-size-image.js

+			const sizesWithoutFullSize = initialSizes.filter( ( size ) => 'full' !== size.slug );
+
+			// If the AMP Story slug isn't present, add it.
+			if ( ! sizesWithoutFullSize.filter( ( size ) => MAX_IMAGE_SIZE_SLUG === size.slug ).length ) {


It'd be nice if this logic to add the image size didn't have to run so often. For example, if it ran only on wp.domReady().

But it looks like the settings update, so these settings will be overwritten if this logic only runs on wp.domReady().

…ably need a better solution There was an error: 447:16 error 'select' is already declared in the upper scope no-shadow

Use the 'image_size_names_choose' filter to make the custom image size available in the media const in amp-story-page/edit.js This prevents making a second request for it.

Also, this enables changing the paramter ownSelect to select.

This is the first iteration, and it might need more information in the notice.

kienstra · 2019-06-06T09:36:07Z

Current Display For Large Video Sizes

bd8d73b adds a notice on selecting a video as the Background Media if it has more than 1 MB per second, like more than 5 MB for a 5-second video.

This might need better styling.
Maybe this could use more information, like the size of the video and its length.

To test this, you could download this image, which is 33 seconds and 252 MB. This should display the notice.

assets/src/stories-editor/blocks/amp-story-page/edit.js

assets/src/stories-editor/components/higher-order/with-replaced-full-size-image.js

includes/class-amp-story-post-type.php

westonruter · 2019-06-06T18:25:26Z

@kienstra I'm getting an error:

Instead of an initial let assignemnt and an if block, this simply assigns it all in one line. Also, simplify reduceRight to reduce. The order doesn't seem to matter, so it doesn't have to be from the right.

As Weston mentioned, it will still work with a large video.

As Weston suggested, the megabytesInBytes constnt can also be used in the admin notice.

As Weston suggested, it's a little less repetetive not having 'AMP' in the name.

If the 'full' size image is never needed in AMP stories, this could be much simpler. It's removed whenever the 'post_type' is 'amp_story'. This might need more thought, though.

kienstra · 2019-06-07T06:09:09Z

@kienstra I'm getting an error:

Thanks for letting me know. With 236e226 removing the class in with-replaced-full-size-image.js, that shouldn't be an issue anymore.

Also, on clicking 'Remove Video,' set that attribute back to undefined.

add_new_max_image_size() actually applies to core Image blocks also.

Use this image size if it's available, otherwise simply default to the full size image.

Before, this didn't work with just the filter for 'image_size_names_choose' so get the correct image size after the actual image is chosen.

The filter didn't have access to the post type, so it would run on posts and pages, needlessly adding the 'amp_story_page' custom size. This approach isn't ideal either, but it won't run outside of AMP stories.

It was defined in the entire module, though also used as a parameter of withSelect(). This isn't ideal, but select() is needed to get the image size.

This seems to help with an issue where getMedia() didn't get the background image before.

I couldn't figure out how to enable a custom size for the background image with purely JS. This won't apply to Image blocks in a non-story editor, like for posts.

kienstra · 2019-06-11T09:49:54Z

Hi @westonruter,
Thanks for your patience here.

0bd76bc#diff-37ed1ba86eaee9e72c2ced620309452bR1545 adds a conditional in the PHP filter to check that 'query-attachments' === $_POST['action'].

This allows it to apply to the background image, but not to the Image block's 'Image Size' <select> element (or an Image block in a non-story editor).

I also brought back the JS filter, like earlier in this PR, which replaces the Image block's Full size with the amp_story_page size.

This time, it didn't see the Uncaught TypeError: Cannot call class as a function error that you found earlier. But it'd be interesting to know if you see that error.

kienstra · 2019-06-11T10:04:29Z

Hi @swissspidy,
Thanks a lot for your help on #2389.

Would you be available to review this again? Thanks, Pascal!

…ide-lighter-media * 'develop' of github.com:ampproject/amp-wp: Update dependency css-loader to v3 (#2563) Consistently import components from block-editor package (#2561) Refactor AMP modes (#2550) Update dependency webpack-cli to v3.3.4 (#2558)

westonruter · 2019-06-11T14:13:43Z

@kienstra I'm getting a JS error upon loading the editor:

kienstra · 2019-06-11T17:02:20Z

@kienstra I'm getting a JS error upon loading the editor:

OK, thanks. The JS filter is probably responsible for that.

…ide-lighter-media * 'develop' of github.com:ampproject/amp-wp: Fix resizing for formatting blocks (#2562) Prevent block invalidation due to missing width/height (#2557)

kienstra · 2019-06-11T22:38:08Z

Nice, great idea: fb29ca8

westonruter

The error is happening here:

Where Component is the extended BlockEdit class:

Based on my limited knowledge of React, I was thinking something like this is needed:

--- a/assets/src/stories-editor/components/higher-order/with-replaced-full-size-image.js
+++ b/assets/src/stories-editor/components/higher-order/with-replaced-full-size-image.js
@@ -19,7 +19,7 @@ const BLOCK_EDITOR_STORE = 'core/block-editor';
 
 export default createHigherOrderComponent(
 	( BlockEdit ) => {
-		return class extends BlockEdit {
+		class BlockEditWithReplacedFullImageSize extends BlockEdit {
 			/**
 			 * The class constructor.
 			 */
@@ -67,7 +67,9 @@ export default createHigherOrderComponent(
 					dispatch( BLOCK_EDITOR_STORE ).updateSettings( { imageSizes: sizesWithoutFullSize } );
 				}
 			}
-		};
+		}
+
+		return ( props ) => <BlockEditWithReplacedFullImageSize { ...props } />;
 	},
 	'withReplacedFullSizeImage'
 );

But that's not making any change. Perhaps its just syntactic sugar for the exact same thing.

westonruter · 2019-06-11T22:45:03Z

assets/src/stories-editor/components/higher-order/with-replaced-full-size-image.js

+				}
+
+				if ( super.componentDidUpdate ) {
+					super.componentDidUpdate();


I'm no React expert, but shouldn't this be passing the args passed into componentDidUpdate? In other words:

super.componentDidUpdate( ...arguments );

Ah, good point.

westonruter · 2019-06-11T22:54:20Z

I don't see any other example in the codebase of BlockEdit extended, so perhaps this is not the right approach. Composition should be used instead?

kienstra · 2019-06-11T22:57:54Z

I don't see any other example in the codebase of BlockEdit extended, so perhaps this is not the right approach. Composition should be used instead?

Good point, inheritance isn't a best practice in React, it seems. The only issue is that this needs to 'hook' into the Image edit component's componentDidUpdate().

But if this extended it with a higher-order-component, the HOC probably wouldn't call its componentDidUpdate method when the Image component's componentDidUpdate method runs.

Maybe there's another approach, though.

kienstra · 2019-06-11T23:05:58Z

I'll keep looking at this.

The JS filter produced a console error. So instead, use the existing PHP filter that Weston had suggested.

kienstra · 2019-06-11T23:23:20Z

Hi @westonruter,
Thanks for your help here.

Does c330ce7 work for you? It uses the PHP filter that you had suggested originally. It works for me locally.

westonruter

This does work!

westonruter · 2019-06-12T04:34:10Z

assets/src/stories-editor/blocks/amp-story-page/edit.js

+									{
+										sprintf(
+											/* translators: %d: the number of recommended megabytes per second */
+											__( 'A video size of less than %d MB per second is recommended.', 'amp' ),


Minor thing but it would be useful to know how many MB/second the selected video is, to know how far from the target the video is.

kienstra · 2019-06-12T04:39:21Z

@westonruter, thanks a lot for your help and patience!

kienstra added 5 commits June 1, 2019 16:16

Begin image changes.

3a0188c

Begin work of guiding users to use lighter media.

35d2bc9

Merge branch 'develop' into update/guide-lighter-media

38bcf6c

Replace the full image size with a custom size

05264e8

Do this on the componentDidUpdate method, as it seems that this is where it is updated. Also, revert a change to webpack.config.js that I accidentally staged before.

Move replaceFullSizeImage into the extending component, from helpers/

3b9d1c0

This is only used on one place, so it is probably better in that class.

googlebot added the cla: yes Signed the Google CLA label Jun 5, 2019

Use the new max image size for the background image, also

c7456fd

Use the newly added 'amp_story_page' image size for the AMP Story background image.

kienstra commented Jun 5, 2019

View reviewed changes

assets/src/stories-editor/blocks/amp-story-page/edit.js Outdated Show resolved Hide resolved

kienstra commented Jun 5, 2019

View reviewed changes

kienstra added 4 commits June 5, 2019 00:26

Address a Travis error by renaming select to ownSelect, but will prob…

dc6447c

…ably need a better solution There was an error: 447:16 error 'select' is already declared in the upper scope no-shadow

Use Weston's suggestion to avoid fetching the media again

78692f6

Use the 'image_size_names_choose' filter to make the custom image size available in the media const in amp-story-page/edit.js This prevents making a second request for it.

Address a Travis error: remove declaration of select

1de5ff8

Also, this enables changing the paramter ownSelect to select.

Add a notice for an excessive video file sizev

bd8d73b

This is the first iteration, and it might need more information in the notice.

swissspidy added the AMP Stories label Jun 6, 2019

westonruter reviewed Jun 6, 2019

View reviewed changes

kienstra added 6 commits June 6, 2019 23:49

Apply Weston's suggestion to simplify isExcessiveVideoSize

d2f9ea1

Instead of an initial let assignemnt and an if block, this simply assigns it all in one line. Also, simplify reduceRight to reduce. The order doesn't seem to matter, so it doesn't have to be from the right.

Change excessive video size Notice to warning, instead of error

4f262f8

As Weston mentioned, it will still work with a large video.

Use the same constant for number of bytes in the notice

972e6af

As Weston suggested, the megabytesInBytes constnt can also be used in the admin notice.

Change the name of the custom image to 'Story Background Maximum Image'

25d1057

As Weston suggested, it's a little less repetetive not having 'AMP' in the name.

Merge branch 'develop' into update/guide-lighter-media

2a0d121

Remove BlockEdit filter for image size in favor of PHP filter

236e226

If the 'full' size image is never needed in AMP stories, this could be much simpler. It's removed whenever the 'post_type' is 'amp_story'. This might need more thought, though.

kienstra added 2 commits June 7, 2019 01:35

Add isExcessiveVideoSize to attributes in index.js

efe1724

Also, on clicking 'Remove Video,' set that attribute back to undefined.

Update a PHP DocBlock description

cf9669a

add_new_max_image_size() actually applies to core Image blocks also.

kienstra changed the title ~~[WIP] Encourage lighter images and videos~~ Encourage lighter images and videos Jun 7, 2019

Use the new custom image size for the post image

1d00672

Use this image size if it's available, otherwise simply default to the full size image.

swissspidy requested a review from westonruter June 10, 2019 19:25

kienstra added 6 commits June 10, 2019 23:34

Merge branch 'develop' into update/guide-lighter-media

e189554

Get the 'amp_story_page' image size for the Background Media

0f8d810

Before, this didn't work with just the filter for 'image_size_names_choose' so get the correct image size after the actual image is chosen.

Add the new max size for the Image block, revert PHP filter

e11fd32

The filter didn't have access to the post type, so it would run on posts and pages, needlessly adding the 'amp_story_page' custom size. This approach isn't ideal either, but it won't run outside of AMP stories.

Address a Travis error relating to select already being defined

7ddba5e

It was defined in the entire module, though also used as a parameter of withSelect(). This isn't ideal, but select() is needed to get the image size.

Store getMedia in a constant

8908fdb

This seems to help with an issue where getMedia() didn't get the background image before.

Reinstate previous PHP filter for the Background image

0bd76bc

I couldn't figure out how to enable a custom size for the background image with purely JS. This won't apply to Image blocks in a non-story editor, like for posts.

kienstra mentioned this pull request Jun 11, 2019

Display a notice if a video in the video block is over 1 MB per second #2560

Closed

kienstra mentioned this pull request Jun 11, 2019

Guide users to not use heavy assets when creating stories #2332

Closed

westonruter added 2 commits June 11, 2019 15:15

Merge branch 'develop' of github.com:ampproject/amp-wp into update/gu…

5040a40

…ide-lighter-media * 'develop' of github.com:ampproject/amp-wp: Fix resizing for formatting blocks (#2562) Prevent block invalidation due to missing width/height (#2557)

Add missing modifierName to createHigherOrderComponent

fb29ca8

westonruter reviewed Jun 11, 2019

View reviewed changes

Use the PHP filter to change the Image block's available sizes

c330ce7

The JS filter produced a console error. So instead, use the existing PHP filter that Weston had suggested.

westonruter approved these changes Jun 12, 2019

View reviewed changes

westonruter merged commit 7409759 into develop Jun 12, 2019

This was referenced Jun 12, 2019

Image as Background Media isn't resized when using Photon #2595

Closed

Poster image sizes aren't right when using Photon #2590

Closed

swissspidy deleted the update/guide-lighter-media branch June 18, 2019 12:22

westonruter mentioned this pull request Aug 11, 2019

Improve story page background media with image srcset, reduced image size, and a11y text #3006

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encourage lighter images and videos #2509

Encourage lighter images and videos #2509

kienstra commented Jun 5, 2019

westonruter commented Jun 5, 2019

kienstra commented Jun 5, 2019

kienstra Jun 5, 2019 •

edited

Loading

kienstra commented Jun 6, 2019

westonruter commented Jun 6, 2019

kienstra commented Jun 7, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter left a comment

westonruter Jun 11, 2019

kienstra Jun 11, 2019

westonruter commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter left a comment

westonruter Jun 12, 2019

kienstra commented Jun 12, 2019

Encourage lighter images and videos #2509

Encourage lighter images and videos #2509

Conversation

kienstra commented Jun 5, 2019

westonruter commented Jun 5, 2019

kienstra commented Jun 5, 2019

kienstra Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

kienstra commented Jun 6, 2019

westonruter commented Jun 6, 2019

kienstra commented Jun 7, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter left a comment

Choose a reason for hiding this comment

westonruter Jun 11, 2019

Choose a reason for hiding this comment

kienstra Jun 11, 2019

Choose a reason for hiding this comment

westonruter commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

kienstra commented Jun 11, 2019

westonruter left a comment

Choose a reason for hiding this comment

westonruter Jun 12, 2019

Choose a reason for hiding this comment

kienstra commented Jun 12, 2019

kienstra Jun 5, 2019 •

edited

Loading