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

Resolve issue with button groups and child variant precedence #1725

Closed
knarkzel opened this issue Jul 4, 2023 · 2 comments
Closed

Resolve issue with button groups and child variant precedence #1725

knarkzel opened this issue Jul 4, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@knarkzel
Copy link

knarkzel commented Jul 4, 2023

Current Behavior

Currently it's only possible to have one style for all the buttons in btn-group.

Expected Behavior

Be able to use different styles for each button in btn-group.

Steps To Reproduce

<div class="btn-group">
  <button class="variant-filled-primary">Monthly</button>
  <button class="variant-ghost-primary">Annually</button>
</div>

image

Link to Reproduction / Stackblitz

https://discord.com/channels/1003691521280856084/1125913854585618534

More Information

As you can see in the screenshot above, the colors are off. --on-primary doesn't work properly, borders aren't grouped etc.

@knarkzel knarkzel added the bug Something isn't working label Jul 4, 2023
@endigo9740
Copy link
Contributor

endigo9740 commented Jul 4, 2023

Appending a bit of additional context here:

We likely won't support mixing and matching button variants within a group as shown above. If the button does not have some unity between the group then they should be split and standalone.

But we are seeing an issue with variant-filled-x not applying the text/fill "on" color when doing something like this:

<div class="btn-group variant-ghost-primary">
  <button class="variant-filled-primary">Monthly</button>
  <button>Annually</button>
</div>

Which could be useful for creating a radio group like grouping that indicates active selection state. It seems the btn-group > button styles are taking precedent for text color, which means the text will flip flop between black/white when toggling light/dark mode instead of adhering to the desired "on" color, which harms accessibility.

I'd assume there's some ! important declaration on the button group styles that is causing this. Likely here:
https://github.com/skeletonlabs/skeleton/blob/dev/packages/plugin/src/styles/components/buttons.css

Also note we likely won't be able to revisit this until the plugin changes in v2 are in place, so this PR is a prerequisite:

@endigo9740 endigo9740 added this to the v2.0 milestone Jul 4, 2023
@endigo9740 endigo9740 modified the milestones: v2.0, Future/Whenever Aug 4, 2023
@endigo9740 endigo9740 changed the title Button groups should be able to have different styles for each button Resolve issue with button groups and child variant precedence Aug 29, 2023
@endigo9740
Copy link
Contributor

endigo9740 commented Sep 8, 2023

Just reiterating that the original post above was not an scenario we were keen to support in Skeleton. If you wish to mix and match variants, create your own button grouping.

However, per having a variant override to indicate state (like a radio field), this appears to be resolved - at least in Skeleton v2. You're now able to supply ! on the variant class to signal that it should take precedence, so like:

<div class="btn-group variant-ghost-primary">
	<button class="!variant-filled-primary">Months</button>
	<button>Days</button>
	<button>Years</button>
</div>

Not only does the text color then work as expected, but the child button with the variant also stays active even when hovering the button. It's a small change that makes this work exactly as expected.

With this, no changes are needed on Skeleton's end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants