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

feat(button): add disabled styles for danger variant #518

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sembauke
Copy link
Member

@Sembauke Sembauke commented Mar 18, 2025

Checklist:

Closes #473

Disabled state:

Screenshot Color contrast
Light Screenshot 2025-03-19 at 13 25 56 https://webaim.org/resources/contrastchecker/?fcolor=A94442cc&bcolor=F2DEDE
Dark Screenshot 2025-03-19 at 13 25 47 https://webaim.org/resources/contrastchecker/?fcolor=F2DEDEcc&bcolor=A94442

@Sembauke Sembauke requested a review from a team as a code owner March 18, 2025 11:42
@Sembauke Sembauke added the release: minor Changes that would go in a minor release. label Mar 18, 2025
Copy link
Member

@huyenltnguyen huyenltnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Sem!

So beside adding styles to the disabled state, we will also need to lift the type restriction as currently the consumers would get a TS error if they write:

<Button variant="danger" disabled>
	Button text
</Button>

For context, we have the restriction in place because we weren't sure how disabled danger and info buttons would look like, and since there weren't any use cases of those, we decided to "turn off" this feature until we have an opportunity to revisit the styles.


Anyway, to lift the restriction, we would need to:

Comment on lines +49 to +51
"border-foreground-danger-disabled",
"bg-background-danger-disabled",
"text-foreground-danger-disabled",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmaxed I'm wondering what you think about this.

We currently only allow the primary button to have disabled state, and when disabled, the button has its opacity set to 80%. This styling approach works for the primary button because the button looks grayed out when disabled. Though it doesn't really work with danger style.

Opacity comparison
100% 80%
Light Screenshot 2025-03-19 at 13 50 29 Screenshot 2025-03-19 at 13 40 28
Dark Screenshot 2025-03-19 at 13 51 44 Screenshot 2025-03-19 at 13 51 51

I don't recommend decreasing the opacity level further, as the current level is already pretty low for a11y.

So I'm wondering if you're okay with this approach (changing the color combination) and if you have any suggestions for the color combination.

@Sembauke
Copy link
Member Author

Thanks for the detailed review @huyenltnguyen,

I am not sure if I got the requested colors correct, could you give it another look?

@huyenltnguyen
Copy link
Member

Sorry, @Sembauke, I got myself confused also 😅

I believe the chosen color combination is red70 and red05. Those colors previously weren't swapped in dark theme, so it looked wrong. My suggestion was to address that, but I think I got wrong as well.

I'll wait for Ahmad to weight in. Though I'll explore some other alternative combinations, too, as the red70 + red05 combination doesn't have good contrast ratio 🫤 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: minor Changes that would go in a minor release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow danger variant to be disabled
2 participants