Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image #8196

danielbachhuber · 2018-07-25T12:44:36Z

Description

Adds a editor.PostFeaturedImage.imageSize filter for the image size displayed in the Post Featured Image component. Modeling the behavior of the existing admin_post_thumbnail_size filter [ref], it defaults to 'post-thumbnail' as the default image size and fails back to the full image size when the specified image size doesn't exist.

It appears the current behavior was defined in #894 (71f1d3a) without any specific discussion about media sizes.

Fixes #7505

How has this been tested?

Open a new Gutenberg post and assign a featured image.
Add the following filter in the Console:

var withImageSize = function() {
	return 'large';
};

wp.hooks.addFilter( 'editor.PostFeaturedImage.imageSize', 'my-plugin/with-image-size', withImageSize );

Close and re-open the Post Featured Image component.
Use the Console Inspector to see that the image size has changed.

Screenshots

Types of changes

Enhancement.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

tofumatt

Makes sense to me but since this is about backward compat and featured images are important: it'd be nice to add a test for this.

tofumatt · 2018-07-25T15:06:41Z

editor/components/post-featured-image/index.js

 	const postLabel = get( postType, [ 'labels' ], {} );

+	let mediaWidth,
+		mediaHeight,


What's up with the indentation here? If these have to be on their own lines it might just be nicer to have separate let declarations. 🤷‍♂️

If these have to be on their own lines it might just be nicer to have separate let declarations

Is there an eslint for this coding standard?

I don't think so, but there aren't any instances of:

let foo, bar, foobar;

in the code when I looked for it. Conversely, there are several instances of multiple-variable let declarations:

https://github.com/WordPress/gutenberg/blob/master/packages/date/src/index.js#L285
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/toggle-control/index.js#L35
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/higher-order/with-focus-outside/test/index.js#L17

My implicit point was: if it's coding style and there's an eslint for it, then it's enforceable. If there isn't a lint for it, then it's a bit perfectionist and detracts from the code review process.

Regardless, I've updated in da643a1

tofumatt · 2018-07-25T15:07:42Z

editor/components/post-featured-image/index.js

+		mediaSourceUrl;
+	if ( media ) {
+		let mediaSize = 'post-thumbnail';
+		mediaSize = applyFilters( 'editor.PostFeaturedImage.imageSize', mediaSize, media.id, currentPostId );


Seems like this could just be const mediaSize = applyFilters( 'editor.PostFeaturedImage.imageSize', 'post-thumbnail', media.id, currentPostId );, then you can ditch the previous line.

tofumatt · 2018-07-25T15:11:18Z

docs/extensibility/extending-blocks.md

@@ -163,6 +163,20 @@ var withDataAlign = wp.compose.createHigherOrderComponent( function( BlockListBl
 wp.hooks.addFilter( 'editor.BlockListBlock', 'my-plugin/with-data-align', withDataAlign );
 ```

+### `editor.PostFeaturedImage.imageSize`
+
+Used to modify the image size displayed in the Post Featured Image component. It defaults to `'post-thumbnail'`, and will fail back to the `full` image size when the specified image size doesn't exist on the media object. It's modeled after the `admin_post_thumbnail_size` filter in the Classic Editor.


I think "exist on the media object" should be "exist in the media object".

danielbachhuber · 2018-07-25T15:52:36Z

it'd be nice to add a test for this.

Can you provide a reference I can follow for this test?

tofumatt · 2018-07-25T17:41:25Z

I would think an e2e test that checked for the output of the cover-image after this filter is applied would be the trick. Not sure if there's much prior art as our tests are still a bit slim... it could be that https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/hooks-api.test.js has examples of how to make hooks work.

I'd imagine what we want here is to test the cover image's output before/after the hook is applied by inserting a cover image block and checking its src, then installing the hook, refreshing the page, and checking the src has changed, right?

danielbachhuber · 2018-07-25T17:44:50Z

I would think an e2e test that checked for the output of the cover-image after this filter is applied would be the trick.

This PR impacts the PostFeaturedImage component?

tofumatt · 2018-07-25T17:54:11Z

Sorry, I meant featured image. I always think of it as a "cover image" in my brain. That's my bad.

danielbachhuber · 2018-07-25T17:56:22Z

Ok. Given the nature of the change, and relative complexity of mocking the test, I'd like to pass on tests for now.

tofumatt

I'm not big on how often we punt on tests; they are a fair amount of effort, especially when there are fewer tests for prior art to draw on, but that means less confidence in our code and in refactors.

The code here is fine, so I'm approving this PR, but we hit regressions in nearly every release and I think the only way to stop playing catch-up is to start writing tests alongside code.

That's not particular to this PR though, it's a systemic issue. No reason to single this PR out on it.

🚢

danielbachhuber · 2018-07-25T18:07:04Z

Thanks @tofumatt. I agree generally with your sentiment about tests; I think the likelihood of tests being written increases with the ease of writing them.

gziolo

Wouldn't it be better to integrate the existing admin_post_thumbnail_size filter and pass its value to Gutenberg?

In the case of similar settings which have some PHP based setup we use editor settings to expose them to the client:

https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1247-L1248

It isn't a perfect approach as noticed by @aduth. However, it helps us to avoid duplicating existing logic. Should we refactor the code merged in this PR to use editor setting or something similar? I think @youknowriad had an idea of extracting media module which could keep those kinds of data.

gziolo · 2018-08-01T08:11:43Z

docs/extensibility/extending-blocks.md

@@ -163,6 +163,20 @@ var withDataAlign = wp.compose.createHigherOrderComponent( function( BlockListBl
 wp.hooks.addFilter( 'editor.BlockListBlock', 'my-plugin/with-data-align', withDataAlign );
 ```

+### `editor.PostFeaturedImage.imageSize`


This documentation should be located in a different place. It has no connection with extending blocks in the current shape.

My bad, can you provide an alternative suggestion?

We should create a new doc file (extending-editor.md?) in the same folder and start adding there all editor related hooks.

danielbachhuber · 2018-08-01T10:30:44Z

Wouldn't it be better to integrate the existing admin_post_thumbnail_size filter and pass its value to Gutenberg?

I decided to create a new JavaScript-based filter, instead of reusing the existing PHP one, because the filter's return value can change based on post state. If Gutenberg were to use the value of the existing admin_post_thumbnail_size filter, then that value would eventually become stale.

This is the same rationale described in #3315 (comment)

Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image

e9e8daf

danielbachhuber added this to the 3.4 milestone Jul 25, 2018

danielbachhuber added [Type] Enhancement A suggestion for improvement. [Feature] Document Settings Document settings experience [Feature] Extensibility The ability to extend blocks or the editing experience Backwards Compatibility Issues or PRs that impact backwards compatability labels Jul 25, 2018

danielbachhuber requested a review from a team July 25, 2018 12:56

danielbachhuber added the [Feature] Media Anything that impacts the experience of managing media label Jul 25, 2018

Include variables passed through for reference

d152ce6

danielbachhuber mentioned this pull request Jul 25, 2018

Featured image meta box does not respect admin_post_thumbnail_size filter #7505

Closed

tofumatt reviewed Jul 25, 2018

View reviewed changes

danielbachhuber added 2 commits July 25, 2018 08:53

Better English

328a08b

Consolidate statement

246b3fa

Move variable definitions to one line

da643a1

tofumatt approved these changes Jul 25, 2018

View reviewed changes

danielbachhuber merged commit 1c2882a into master Jul 25, 2018

danielbachhuber deleted the 7505-filter-image-size branch July 25, 2018 18:07

gziolo reviewed Aug 1, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image #8196

Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image #8196

danielbachhuber commented Jul 25, 2018 •

edited

Loading

tofumatt left a comment

tofumatt Jul 25, 2018

danielbachhuber Jul 25, 2018

tofumatt Jul 25, 2018

danielbachhuber Jul 25, 2018

tofumatt Jul 25, 2018

danielbachhuber Jul 25, 2018

tofumatt Jul 25, 2018

danielbachhuber Jul 25, 2018

danielbachhuber commented Jul 25, 2018

tofumatt commented Jul 25, 2018

danielbachhuber commented Jul 25, 2018

tofumatt commented Jul 25, 2018

danielbachhuber commented Jul 25, 2018

tofumatt left a comment

danielbachhuber commented Jul 25, 2018

gziolo left a comment

gziolo Aug 1, 2018

danielbachhuber Aug 1, 2018

gziolo Aug 1, 2018

danielbachhuber Aug 1, 2018

danielbachhuber commented Aug 1, 2018

Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image #8196

Introduce 'editor.PostFeaturedImage.imageSize' filter for Featured Image #8196

Conversation

danielbachhuber commented Jul 25, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jul 25, 2018

tofumatt commented Jul 25, 2018

danielbachhuber commented Jul 25, 2018

tofumatt commented Jul 25, 2018

danielbachhuber commented Jul 25, 2018

tofumatt left a comment

Choose a reason for hiding this comment

danielbachhuber commented Jul 25, 2018

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Aug 1, 2018

danielbachhuber commented Jul 25, 2018 •

edited

Loading