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

Add Image Size dropdown for Media & Text block #24795

Merged

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Aug 25, 2020

Closes: #24688

Description

Add an Image Size dropdown for Media & Text block. We can change the size of the image for the Image block, but we didn't have the option to do the same for Media & Text block. Which caused the Media & Text block to always render the full-size image.

NOTE: Default value for Image Size is Full. I think Large would be better in general, but that would break existing posts.

How has this been tested?

  1. Open Editor
  2. Add Media & Text Block
  3. Select an image from the media library
  4. Make sure Media & Text Block is in focus
  5. You should see an Image Size dropdown in the right sidebar

Screenshots

image

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@david-szabo97 david-szabo97 marked this pull request as ready for review August 26, 2020 07:58
@david-szabo97 david-szabo97 added [Type] Feature New feature to highlight in changelogs. [Block] Media & Text Affects the Media & Text Block labels Aug 26, 2020
@david-szabo97
Copy link
Member Author

@ockham Thanks for the feedback! Replaced them with shorthand.

@jeyip
Copy link
Contributor

jeyip commented Aug 31, 2020

I'm not sure this is behaving as I'd expect. It looks like the "Thumbnail, Medium, Large, Full" dropdown only modifies the media resolution, and not the media dimensions (Ex. Width / Height). I'm still wrapping my head around the block editor user experience and all of its quirks, so someone else may have more insight. Maybe @ockham ? 🙂

Media & Text Block Image Block
Media and Text Image

@david-szabo97
Copy link
Member Author

@jeyip Actually, that's the correct behaviour here. The dimensions of the media is controlled by the Media & Text Block. Might seem strange but the image always fits it's cell so the Image Size won't affect the dimensions.

@jeyip
Copy link
Contributor

jeyip commented Sep 1, 2020

I see why, from a technical perspective, the dimensions don't change.

I, however, as a WordPress user, would expect the image to resize appropriately. The reason I think this is because if the images don't resize, the only feature this introduces is the ability to display a set of more or less pixelated images. Although there might be a use case, I don't think this captures the intention of the original issue. @nosolosw or @deskoj would either of you be able to speak more about your expectations?

@Copons
Copy link
Contributor

Copons commented Sep 2, 2020

I think this has lots of value, but I agree that "image size" sets the wrong expectation.
What about "image quality" instead?

@david-szabo97
Copy link
Member Author

I'm not sure what would be the best approach to present this to the user. It's more like manual image optimization since the block itself can't decide which size of the image should be used.

What about "image quality" instead?
I think this would make sense!

@jeyip
Copy link
Contributor

jeyip commented Sep 2, 2020

I think this has lots of value, but I agree that "image size" sets the wrong expectation.
What about "image quality" instead?

I think this would make sense!

I think renaming it image quality makes a lot of sense too 👍

I'm still curious about the value proposition, and I think I might be missing something. @david-szabo97 or @Copons would you mind elaborating a little bit about the use case for image quality? (Thank you ahead of time for the patience 🙏 ) Is it to improve frontend loading speed?

The code looks good, so this has my +1 when we resolve this conversation.

@Copons
Copy link
Contributor

Copons commented Sep 2, 2020

I'm still curious about the value proposition, and I think I might be missing something. @david-szabo97 or @Copons would you mind elaborating a little bit about the use case for image quality? (Thank you ahead of time for the patience 🙏 ) Is it to improve frontend loading speed?

Yeah, pretty much.
Let's say your content is 700px wide, and the image in the Media & Text is half that, 350px.
You upload a 1 bazillion megapixel image. The block uses it at full size, causing visitors to hit their monthly data limit, when a smaller version of that image would have done just as nicely.
(It should also improve the editor speed, but I'd have to double check this.)

It's also good to have the same options for similar features.
It makes the UX less inconsistent, especially in consideration of the fact that users does not need to know or understand that Image and Media & Text are different under the hood.

@jeyip
Copy link
Contributor

jeyip commented Sep 3, 2020

Let's say your content is 700px wide, and the image in the Media & Text is half that, 350px.
You upload a 1 bazillion megapixel image. The block uses it at full size, causing visitors to hit their monthly data limit, when a smaller version of that image would have done just as nicely.

Thanks for the explanation. Much appreciated :)

@david-szabo97
Copy link
Member Author

@jeyip @Copons

I think renaming it image quality makes a lot of sense too 👍

+1, what do you think about merging this and I'll follow-up with a PR for the rename?

The label is hardcoded, so I'd make it configurable via prop in a separate PR.

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

Sure @david-szabo97 sounds good to me! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media & Text: set image size
4 participants