Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media: Use image default size from settings #29966

Merged
merged 2 commits into from
Mar 24, 2021

Conversation

Mamaduka
Copy link
Member

Description

Allows for user-defined image default size setting to be used instead of hardcoded one.

The setting will fallback to the default value, when

  • image_default_size option isn't defined.
  • The option doesn't match any registered image size slug for the block editor.

Fixes #8663, #15091 and #20269.

How has this been tested?

Manually

  1. Set image_default_size option to "medium" on wp-admin/options.php page.
  2. Insert new image block.
  3. Media size should be "Medium" by default.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • [N/A] I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@Mamaduka Mamaduka added [Feature] Media Anything that impacts the experience of managing media [Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement. labels Mar 18, 2021
@Mamaduka Mamaduka self-assigned this Mar 18, 2021
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I haven't tested but it looks solid. Is there a related ticket opened in WordPress Trac?

This PR needs a rebase.

packages/block-library/src/image/edit.js Outdated Show resolved Hide resolved
$image_default_size = get_option( 'image_default_size', 'large' );
$image_sizes = wp_list_pluck( $settings['imageSizes'], 'slug' );

$settings['imageDefaultSize'] = in_array( $image_default_size, $image_sizes, true ) ? $image_default_size : 'large';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there is a few places where those settings are filtered ...

Maybe, it would be better to keep them next to imageSizes:

'imageSizes' => $available_image_sizes,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like gutenberg_get_common_block_editor_settings isn't used for post editor. Only for site, widgets, and navigation.

I think it makes sense to keep this in the gutenberg_extend_post_editor_settings function for now. Saying this based on DocBlocks of both functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, it's good to keep as is then. I will investigate separately what's the role of gutenberg_get_common_block_editor_settings. FSE and Global Styles add a ton of complexity in the plugin on the PHP side until they are moved to WordPress core.

@Mamaduka Mamaduka force-pushed the add/image-default-size-setting branch from 640231f to 44ea521 Compare March 23, 2021 15:35
@Mamaduka
Copy link
Member Author

Is there a related ticket opened in WordPress Trac?

I didn't know I should create one. Is there a contributor guideline when we should create related WordPress Trac tickets?

I've rebased PR. But it looks like checks are failing for unrelated reseasons.

@gziolo
Copy link
Member

gziolo commented Mar 23, 2021

Is there a related ticket opened in WordPress Trac?
I didn't know I should create one. Is there a contributor guideline when we should create related WordPress Trac tickets?

I don't know if there are guidelines, but it's simple to explain. Whenever there is a PHP change that changes/extends the existing behavior in WordPress core it should be backported at some point.

@Mamaduka
Copy link
Member Author

Makes sense. I will create a related Trac ticket as well. Thanks for mentioning this.

@gziolo
Copy link
Member

gziolo commented Mar 23, 2021

Related code in the WP Core:
https://github.com/WordPress/wordpress-develop/blob/d271a206dbc87421b221edb2f7acff6ea523fb5d/src/wp-admin/edit-form-blocks.php#L308-L343

@paaljoachim
Copy link
Contributor

I made a special download of the Gutenberg plugin. By going to the tab "Checks" (just below the title).
Clicked the Build Gutenberg plugin in the sidebar.
Then downloaded the build as a plugin.
Unzipped it and zipped it up again.
Then uploaded and activated on a local test site.

I added an image and changed the Image size in the sidebar to Medium.
Then add another image to the same page and expected it to automatically have it preselected as Medium.
That did not happen.

@Mamaduka
Copy link
Member Author

Hi, @paaljoachim

This PR doesn't add the feature to remember the last selected image size. Instead, it will use user-specified image default size from settings. So it partially fixes #8663.

You can test it using the following steps:

  1. Set image_default_size option to "medium" on wp-admin/options.php page.
  2. Insert new image block.
  3. Media size should be "Medium" by default.

@gziolo, it looks like all the checks are passing now. Is it okay to merge this?

@gziolo
Copy link
Member

gziolo commented Mar 23, 2021

it looks like all the checks are passing now. Is it okay to merge this?

Yep, good to go based on your previous comment clarifying the behavior.

@Mamaduka Mamaduka merged commit eacc781 into WordPress:trunk Mar 24, 2021
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 24, 2021
@Mamaduka Mamaduka deleted the add/image-default-size-setting branch March 24, 2021 03:24
@Mamaduka
Copy link
Member Author

Core track ticket: https://core.trac.wordpress.org/ticket/52896.

@feastdesignco
Copy link

@Mamaduka you're my hero.

nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Mar 24, 2021
Related: WordPress/gutenberg#29966

Add new imageDefaultSize value to block editor settings.

Props Mamaduka.
Fixes #52896.




git-svn-id: https://develop.svn.wordpress.org/trunk@50570 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Mar 24, 2021
Related: WordPress/gutenberg#29966

Add new imageDefaultSize value to block editor settings.

Props Mamaduka.
Fixes #52896.




git-svn-id: https://develop.svn.wordpress.org/trunk@50570 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Mar 24, 2021
Related: WordPress/gutenberg#29966

Add new imageDefaultSize value to block editor settings.

Props Mamaduka.
Fixes #52896.



Built from https://develop.svn.wordpress.org/trunk@50570


git-svn-id: http://core.svn.wordpress.org/trunk@50183 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Mar 24, 2021
Related: WordPress/gutenberg#29966

Add new imageDefaultSize value to block editor settings.

Props Mamaduka.
Fixes #52896.



Built from https://develop.svn.wordpress.org/trunk@50570


git-svn-id: https://core.svn.wordpress.org/trunk@50183 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@hypest
Copy link
Contributor

hypest commented Mar 24, 2021

👋 friends, looks like this PR introduced a regression in the native mobile block editor. A fix PR is already in the making.

  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

Looking at the changes and the fix, I understand that the regression could have been caught by noticing that DEFAULT_SIZE_SLUG is been used in native.js files so by removing it there would be an issue introduced. Since the PR description checklist already includes a check item exactly for that kind of changes, I wonder if there's been some discussion around why that check item is was not relevant in this PR (since I see it marked as "N/A"). Thanks!

@Mamaduka
Copy link
Member Author

Mamaduka commented Mar 24, 2021

Hi, @hypest

Sorry, that's totally my fault. I should have searched block dir manually instead of just trusting the linters.

Is there documentation on how I can test the native mobile block editor? To avoid similar regression in the future.

Thanks,

George

@hypest
Copy link
Contributor

hypest commented Mar 24, 2021

Thanks for the understanding @Mamaduka ❤️ .

I think the PR description checklist item can use some rephrasing (and we'll do that in a separate PR. EDIT: here's the PR) as I understand it might not be easy to know which files are those and how one should go about checking them. Sorry for that.

Is there documentation on how I can test the native mobile block editor? To avoid similar regression in the future.

I appreciate the empathy here 🙇‍♂️. There's this React Native based mobile Gutenberg guide linked from the Code contributions guide. It's a bit hard to reach, that's fair, but it can serve as a good starting point. On top of that, always happy to help or give a hand so feel free to ping me or @WordPress/native-mobile. Thanks! 🙇‍♂️

@feastdesignco
Copy link

Bump. This continues to be a major ongoing issue.

@Mamaduka
Copy link
Member Author

Hi, @feastdesignco

Can you provide more details?

This PR got merge into Gutenberg core a while ago, and now it should be part of WP 5.8 as well.

F-Wilke pushed a commit to FiliagoDev/WordPress that referenced this pull request Jul 31, 2021
Related: WordPress/gutenberg#29966

Add new imageDefaultSize value to block editor settings.

Props Mamaduka.
Fixes #52896.



Built from https://develop.svn.wordpress.org/trunk@50570


git-svn-id: http://core.svn.wordpress.org/trunk@50183 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@bilalmalkoc
Copy link

I think it should be available for gallery block also.

@feastdesignco
Copy link

Hi, @feastdesignco

Can you provide more details?

This PR got merge into Gutenberg core a while ago, and now it should be part of WP 5.8 as well.

Looks like it's functioning now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Size: Remember the last size used.
6 participants