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

Post Featured Image Aspect Ratio: Link width and height controls #47963

Closed
scruffian opened this issue Feb 10, 2023 · 6 comments
Closed

Post Featured Image Aspect Ratio: Link width and height controls #47963

scruffian opened this issue Feb 10, 2023 · 6 comments
Labels
[Block] Post Featured Image Affects the Post Featured Image Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.

Comments

@scruffian
Copy link
Contributor

What problem does this address?

Since we merged #47854, the width and height controls work independently of the aspect ratio control and it's not clear how they interact.

Here's a suggestion from @richtabor:

I'm thinking that if an aspect ratio is assigned, then the height/width should adhere to that ratio (regardless of which is changed). For example, if you set the aspect ratio to 16:10, then set a width of 1600px, the height should be auto-set to 1000px. And if you change the height to 800px, then the width would auto-set to 1280 (maintaining the 16:10 aspect ratio).

We could also leverage the existing link/unlink control to show that the values are linked together (and provide a way to unlink the height/width). If unlinked, then the aspect ratio value should no longer represent the chosen ratio, but rather "Freeform" (or another term if we prefer).

CleanShot.2023-02-09.at.16.49.36.mp4
@scruffian scruffian added the [Block] Post Featured Image Affects the Post Featured Image Block label Feb 10, 2023
@scruffian scruffian changed the title Image Aspect Ratio Post Featured Image Aspect Ratio: Link to width and height controls. Feb 10, 2023
@richtabor richtabor added the Needs Design Feedback Needs general design feedback. label Feb 10, 2023
@jasmussen
Copy link
Contributor

I like it.

  • There's something about the 3 oddly sized cover/contain/fill that feel uneven, and honestly, "Fill" doesn't feel that compelling to me, I can't think of when I'd actually use this.
  • The link/unlink makes sense but I don't love how its placement breaks the otherwise fairly strict 2 column grid that we're aiming for (see also UI Components, settle on 40px and 32px "small" sizes. #46734).

I took a quick stab at a remix:

Frame 1

It's essentially the same mockup, but the width/height becomes a single control rather than two separate ones, so it can have a single label and the link/unlink on the right. This is the same pattern the Font size control uses with the "settings" icon on the right.

It omits the "Fill" option, leaving the two scaling options to breathe. There's a question whether a segmented control is still appropriate when there are only two options, the alternative could be a toggle. But I like how it affords good help-text. What do you think?

@richtabor
Copy link
Member

What do you think?

Love it.

There's a question whether a segmented control is still appropriate when there are only two options, the alternative could be a toggle.

I think in this case it does track better than a toggle would, especially with the more technical nature of the attribute and the amount of help text.

Should it be Width & Height, or Height & Width?

The current control in the core/post-featured-image block is Height/Width, whereas the core/image block is Width/Height.

@jasmussen
Copy link
Contributor

I picked width and height to match the sequence of the fields, but no strong opinion. Can also pair it with onfocus tooltips a la the exploded border control if need be. Also yes, it'd be nice to revisit the featured image control and harmonize 👍 👍

Nice work!

@scruffian
Copy link
Contributor Author

Width and height is the correct order according to the rules of ablaut reduplication.

@amjadr360
Copy link

I'm thinking that if an aspect ratio is assigned, then the height/width should adhere to that ratio (regardless of which is changed). For example, if you set the aspect ratio to 16:10, then set a width of 1600px, the height should be auto-set to 1000px. And if you change the height to 800px, then the width would auto-set to 1280 (maintaining the 16:10 aspect ratio).

With this behavior, users will face unwanted width and height image size issues. Such as, if the user sets the image size as 2000px and wants to set the image aspect ratio to 16:10, it will result in the image size of 1600px, but the user wants 2000px size image.

I think the aspect ratio should not affect the width/height of the image.

@richtabor
Copy link
Member

richtabor commented Oct 4, 2023

I don't think we need this anymore, with the current implementation of using "Auto".

Frame 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Featured Image Affects the Post Featured Image Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

6 participants