WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.54k stars 4.21k forks source link

Move Social Icons size setting to the inspector #65647

Open afercia opened 2 months ago

afercia commented 2 months ago

Description

The Social Icons block has a 'size' setting that is placed in a toolbar dropdown menu labeled 'Size':

Screenshot 2024-09-25 at 12 40 06

Screenshot 2024-09-25 at 14 02 44

I may be missing something but I couldn't think of any other 'size' setting placed in the block toolbar. All the 'size' settings I can think of are placed in the settings panel (the Inspector). The Buttons block is very similar to the Social Icons one, which has its Size setting in the Insepctor. Other size settings are in the Inspector, whether they're typograhy global sizes or the font size picker for some blocks.

As a user, I learnt to associate the concept of 'size' with the setting panel and I'd expect to find this setting always in the settings panel.

Is there any design reason to place the Social Icons size setting in the block toolbar? To me, this seems an unique case, an inconsistency that doesn't help provide a cohesive, consistent experience to users.

Cc @WordPress/gutenberg-design

Step-by-step reproduction instructions

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Please confirm that you have tested with all plugins deactivated except Gutenberg.

hanneslsm commented 1 month ago

I may be missing something but I couldn't think of any other 'size' setting placed in the block toolbar.

Not size, but the search block has also display options in the toolbar. (Something that should be revisited also, though.)

richtabor commented 1 month ago

Curious, why is this labeled for accessibility?

richtabor commented 1 month ago

I agree, there's potential in moving this to the inspector (or potentially having both placements—not 100% sure on that though).

Are you thinking a ToggleGroup control, similar to font size?

afercia commented 1 month ago

Not size, but the search block has also display options in the toolbar. (Something that should be revisited also, though.)

@hanneslsm good point. Would you like to create an issue for the Search block when you have a chance? I'd suggest the design team to audit all 'display' settings for all blocks though, as I think there's more cases where the placement of settings related to 'display' is largely inconsistent. @WordPress/gutenberg-design

afercia commented 1 month ago

Looking a bit more into this, I'd think that before making any decision on the component to use, an evaluation should be made to consider whether the Size setting should evolve to a setting controlled by the theme. To my understanding, the current values are hardcoded:

https://github.com/WordPress/gutenberg/blob/0ae80bcb8de64df1b5b71d07191802b8c51fbf74/packages/block-library/src/social-links/edit.js#L33-L38

I'd think themes should be able to provide their own size values. In that case, the values may be more than 4, which is important to determine the control ot use. For example, the Font size picker is a toggle group and changes to a CustomSelectControl when the theme provides more than 5 values.

Also, I'd think users should be allowed to set a custom size.

t-hamano commented 1 month ago

Rather than the theme providing presets, what if we just let the RangeControl apply any size we want, like for example the Site Logo block?

image

afercia commented 1 month ago

Using the ToggleGroupControl as done in https://github.com/WordPress/gutenberg/pull/65729 doesn't seem ideal to me. The ToggleGroupControl is designed for very short labels like the t-shirt sizes. It really doesn't work well with longer labels. Also, I'm not sure I understand why the social icons should only have 4 sizes.

Screenshot in Spanish, Portuguese, Dutch:

Screenshot 2024-10-01 at 09 15 48

The RangeControl is meant for values in a range. Potentially, users may want to use a value that is out of the provided range. To me, the RangeControl should only be used when there is a fixed, known, range. That means the usage of RangeControl should be audited across the whole UI including the Site Logo.

To me, this setting should allow users to set whatever size value they want. I think a UnitControl is the most appropriate one.

t-hamano commented 1 month ago

Allowing custom values ​​is a bit complicated because we need to consider backward compatibility.

Can't we address this in a follow-up after moving the settings to the Inspector? However, if there is no other suitable component than the UnitControl component, we will need to address it at the same time.

afercia commented 1 month ago

Can't we address this in a follow-up after moving the settings to the Inspector?

No objections from me but the ToggleGroupControl doesn't look right to me.

t-hamano commented 1 month ago

Another approach is to use t-shirt sizes in a ToggleGroupControl component. This means that the labels change but the internal values ​​remain the same. However, users may misunderstand that the values ​​defined in the font size preset also apply to the social icons.

image

afercia commented 1 month ago

The t-shirt sizes pattern is less than ideal for accessibility as the visible lable mismatch the actual accessible name. It's just a wrong pattern that hasn't been designed with accessibility in mind and should be entirely retired in the future. There are also other technical issues with that pattern. For example, themes can provide their own custom font sizes and _names. A font size with name 'Tiny' would anyway get one of the t-shirt sizes labels e.g:

It's just that the t-shirt sizes are assigned in a fixed order and can't be changed, regardless of the actual size names, which is a less-than-ideal implementation.

t-hamano commented 1 month ago

I am not talking about font size UI. I mean building our own ToggleGroupControl in the Social Link block. In the UI I proposed, I do not assume that tooltips will be displayed, and there will be no mismatch between the label and the tooltip. What I am proposing is simply to use shorter labels so that the ToggleGroupControl does not cause layout misalignment.

afercia commented 1 month ago

I do not assume that tooltips will be displayed, and there will be no mismatch between the label and the tooltip

@t-hamano thanks for clarifying. So basically you are suggesting to use the t-shirt sizes names without a tooltip. That would introduce an inconsistency in the usage of the ToggleGroupControl that is hard to justify and I'm not sure it woul dbe a good useer experience.

Screenshot 2024-10-02 at 08 46 09

That way, the Font Size picker and the Social Icons size setting would basically look the same. However, despite their similar visuals, the tooltip behavior and more importantly the accessible name would be inconsisteng.

Visually, both will whos the t-shirt sizes abbreviations: S, M, L, etc.

However, the accessible names would be different: Font Size Picker: Small, Medium, Large, Extra Large, Extra Extra Large Social Icons size: S, M, L, XL

While the visual issue would be solved, we always need to think also at semantics and labeling. In this case, the inconsistent labeling wouldn't be ok.

Lastly, a ToggleGroupControl would also need a way to 'reset' to the defautl value, much like the font size picker.

afercia commented 1 month ago

On a related note: why in other cases that are conceptually similar the editor is using different controls? For example, the Image block 'resolution' is basically what users will associate to the concept of 'size', but it uses a select:

Screenshot 2024-10-02 at 09 11 22

I think that for consistency and a more cohesive user experience, all settings related to the concept of 'size' should use the same UI. I'll create a new issue.

t-hamano commented 1 month ago

a ToggleGroupControl would also need a way to 'reset' to the defautl value, much like the font size picker.

The ToggleGroupControl component itself has an option to deselect it (isDeselectable prop).

However, the text options + isDeselectable pattern does not seem to be recommended (See this comment).

Furthermore, I don't think a reset or deselect option is necessary for now, since the current Size option don't allow deselection.

This is one of the reasons I suggested SelectControl component.

jameskoster commented 1 month ago

The RangeControl is meant for values in a range. Potentially, users may want to use a value that is out of the provided range. To me, the RangeControl should only be used when there is a fixed, known, range

This sounds identical to SpacingSizesControl, this one:

It's a range with (theme-supplied) defined steps. But users can elect to use a totally custom value. Would that pattern work here?

afercia commented 1 month ago

This sounds identical to SpacingSizesControl

That's basically a SpacingInputControl that renders with differently based on some conditions. What I don't like much of the SpacingInputControl is that is a block-editor ad-hoc implementation that was designed for spacing settings. I'd think there's the need for a more generic component for the concept of sizes.

Also, I'd like to mention that there is some inconsistency in the 'size' settings across the editor. From an user persepctive, the image resolution is an image 'size'. However, the setting uses a select:

Image

I would like the editor to always provide the same control for a setting conceptually related to 'size'.

jameskoster commented 1 month ago

I'd think there's the need for a more generic component for the concept of sizes.

That's what I was thinking; a new low-level component that accepts a preset range of values and displays them as a stepped range UI accordingly. It would include an option to allow users to set a custom value. SpacingSizesControl would be a consumer of this component, and it could be used in the Social Icons block inspector too.

I would like the editor to always provide the same control for a setting conceptually related to 'size'.

Generally I agree but there can be nuance. The image size control you reference doesn't always affect the rendering size, so feels a bit different to me. Additionally the ergonomics of range controls (drag support, single-click setting) can be superior to selects, especially when there are only a few options.

t-hamano commented 1 month ago

I'd think there's the need for a more generic component for the concept of sizes.

Here are the related issues/PRs:

afercia commented 1 month ago

Generally I agree but there can be nuance. The image size control you reference doesn't always affect the rendering size, so feels a bit different to me.

Yes I'd agree it's a little different, still I'd love to see the same control used for all 'size' settings.

Additionally the ergonomics of range controls (drag support, single-click setting) can be superior to selects

Why? :) A select elemeent is native and it has all the usability and accessibility support we need. I wouldn't say the same for a range control.

especially when there are only a few options.

Which brings us back to the fact we're uaneble to predict how many values a size control will have. If it's a few, than the toggle group works well. If it's more than a few, neither the toggle group and a range would work well.

t-hamano commented 1 month ago

It appears that the discussion here has grown much larger than the scope of this issue was originally proposed.

Perhaps implementing custom values ​​and consistent size controls across the project would take a long time and seems to require its own issue.

Could this issue simply focus on moving the settings to the InspectorControl?

afercia commented 1 month ago

ould this issue simply focus on moving the settings to the InspectorControl?

Yes please, as long as a new issue is created to continue the discussion or, maybe better, we change the title and scope of this issue to not lose references to all the feedback and considerations provided here.

t-hamano commented 1 month ago

While the discussion happening here has been very useful, I personally don't prefer to change the title, purpose, or scope of the original issue. This is because it could confuse someone seeing this issue for the first time as they read the comments from the top of the page.

I would be happy to just discuss moving size setting to the inspector here, and continue the discussion of custom values ​​and new components in a separate issue.