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 styling for disabled options in ToggleGroupControl and try polish #34945

Closed
wants to merge 1 commit into from

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Sep 20, 2021

While working on #34893, I found that ToggleGroupControl has no styling for disabled options. It gets an inline pointer-events: none thanks to Reakit but that's it. So mainly this PR is about adding some basic styles to help differentiate disabled states. I think the difference may be too subtle but I'd like to get some feedback.

Besides that there are two more small changes that I think are improvements.

  • The selected indicator can be a bit smudgy (probably difficult to discernible on 2x+ screens). In order to sharpen it up it needs to be sized and placed on whole pixels so the code is revised for integers and it ends up a bit simplified too.
  • While looking around the component I found myself confused about the purpose of LabelPlaceholderViewbecause it is both visually and aria-hidden. I removed it and still can't figure out what it does. I will not be surprised if I'm missing something and have to restore this though.

How has this been tested?

Manually

Screenshots

Disabled Styles

Options 1 & 2 are disabled

Before After
image image

Post Featured Image block inspector

Before: smudgy After: crispy
image image
image image

Types of changes

Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).
@stokesman
Copy link
Contributor Author

Maybe I'll have reason to pick this up someday but for now…

@stokesman stokesman closed this Jun 12, 2024
@ciampo
Copy link
Contributor

ciampo commented Jul 11, 2024

The selected indicator can be a bit smudgy (probably difficult to discernible on 2x+ screens). In order to sharpen it up it needs to be sized and placed on whole pixels so the code is revised for integers and it ends up a bit simplified too.

This aspect should be hopefully fixed by @DaniGuardiola as he's about to rewrite the indicator animation 🤞

ToggleGroupControl` has no styling for disabled options. It gets an inline pointer-events: none thanks to Reakit but that's i

Regarding this, we could get some direction from @WordPress/gutenberg-design and take it from there (no rush, though!)

@jasmussen
Copy link
Contributor

Feel free to open an issue for a disabled state for this component. If we have a context in the editor, even better. I can probably think up some cases where a disabled segmented control would make sense, but I'd love to hear more thoughts. In most cases I would think the segmented control should not be used in flows where it would need to be disabled. That's not that we shouldn't handle the case at the component level. Just thinking more in terms of: is this just an opacity change, or does it need more thought.

@ciampo
Copy link
Contributor

ciampo commented Jul 11, 2024

The component doesn't currently have any disabled states — so probably adding an opacity change + disabled cursos is already an improvement.

We can always refine these styles at a later point if need be

@jasmussen
Copy link
Contributor

Yep, I think the toggle is just low opacity when disabled, that works well. Can try that for this too.

@ciampo
Copy link
Contributor

ciampo commented Jul 11, 2024

While I tested the current status of disabled option styles, I noticed that disabled options just don't work well. Opened #63450 to add styles and fix the disabled behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
3 participants