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

Move Social Icons size setting to the inspector #65647

Open
2 tasks done
afercia opened this issue Sep 25, 2024 · 23 comments · May be fixed by #65729
Open
2 tasks done

Move Social Icons size setting to the inspector #65647

afercia opened this issue Sep 25, 2024 · 23 comments · May be fixed by #65729
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Sep 25, 2024

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

  • Add a Social Icons block.
  • Observe the Size setting is in the block toolbar.
  • Add a Buttons block.
  • Observe the Size setting is in the settings panel (the Inspector).
  • Observe other 'Size' settings are placed in the settings panel (the Inspector).

Screenshots, screen recording, code snippet

No response

Environment info

No response

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

  • Yes

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

  • Yes
@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block library /packages/block-library [Block] Social Affects the Social Block - used to display Social Media accounts labels Sep 25, 2024
@hanneslsm
Copy link

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
Copy link
Member

Curious, why is this labeled for accessibility?

@richtabor
Copy link
Member

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 afercia added the Needs Design Feedback Needs general design feedback. label Sep 30, 2024
@afercia
Copy link
Contributor Author

afercia commented Sep 30, 2024

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
Copy link
Contributor Author

afercia commented Sep 30, 2024

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:

const sizeOptions = [
{ name: __( 'Small' ), value: 'has-small-icon-size' },
{ name: __( 'Normal' ), value: 'has-normal-icon-size' },
{ name: __( 'Large' ), value: 'has-large-icon-size' },
{ name: __( 'Huge' ), value: 'has-huge-icon-size' },
];

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.

@vipul0425 vipul0425 linked a pull request Sep 30, 2024 that will close this issue
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 30, 2024
@t-hamano
Copy link
Contributor

t-hamano commented Oct 1, 2024

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
Copy link
Contributor Author

afercia commented Oct 1, 2024

Using the ToggleGroupControl as done in #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
Copy link
Contributor

t-hamano commented Oct 1, 2024

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
Copy link
Contributor Author

afercia commented Oct 1, 2024

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
Copy link
Contributor

t-hamano commented Oct 1, 2024

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
Copy link
Contributor Author

afercia commented Oct 1, 2024

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:

  • Viusal label: S.
  • Actual accessible name and tooltip: Tiny.

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
Copy link
Contributor

t-hamano commented Oct 1, 2024

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
Copy link
Contributor Author

afercia commented Oct 2, 2024

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
Copy link
Contributor Author

afercia commented Oct 2, 2024

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
Copy link
Contributor

t-hamano commented Oct 3, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

afercia commented Oct 7, 2024

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
Copy link
Contributor

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
Copy link
Contributor

t-hamano commented Oct 7, 2024

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

Here are the related issues/PRs:

@afercia
Copy link
Contributor Author

afercia commented Oct 7, 2024

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
Copy link
Contributor

t-hamano commented Oct 7, 2024

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
Copy link
Contributor Author

afercia commented Oct 7, 2024

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
Copy link
Contributor

t-hamano commented Oct 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Block library /packages/block-library [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants