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.

Sign up for GitHub

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

Jump to bottom

fix(colors v4): 6832 - Update destructive-foreground color to avoid overlap #6846

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

fix(colors v4): 6832 - Update destructive-foreground color to avoid overlap #6846

wants to merge 2 commits into from
+6 −3

Conversation

Jacksonmills
Copy link
Contributor

@Jacksonmills Jacksonmills commented Mar 4, 2025 •
edited
Loading

Changes the destructive-foreground CSS variable to the correct color to prevent it from matching the destructive color, ensuring better visual differentiation.

image
image

Fixes #6832

@Jacksonmills
fix(colors): 6832 - Update destructive-foreground color to avoid ov…
dc08097 
…erlap

Changes the `destructive-foreground` CSS variable to the correct color to prevent it from matching the `destructive` color, ensuring better visual differentiation.

Fixes shadcn-ui#6832
@Jacksonmills
Merge branch 'main' into fix/6832-destructive-foreground-is-same-colo…
dcb860c 
…r-as-destructive
@vercel Vercel
Copy link

vercel bot commented Mar 4, 2025

@Jacksonmills is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@Jacksonmills Jacksonmills changed the title fix(colors): 6832 - Update destructive-foreground color to avoid overlap fix(colors v4): 6832 - Update destructive-foreground color to avoid overlap Mar 4, 2025
@vercel Vercel
Copy link

vercel bot commented Mar 4, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview Mar 4, 2025 5:54am
v4 ✅ Ready (Inspect) Visit Preview Mar 4, 2025 5:54am

@shadcn
Copy link
Collaborator

shadcn commented Mar 4, 2025

@Jacksonmills This is breaking alert destructive, form error messages...etc everywhere text-destructive-foreground is used.

@Jacksonmills
Copy link
Contributor Author

@Jacksonmills This is breaking alert destructive, form error messages...etc everywhere text-destructive-foreground is used.

Hm interesting, in v3 destructive-foreground was white-ish, so the change to red was intended?

@Jacksonmills
Copy link
Contributor Author

@shadcn I think it's redundant for destructive and destructive-foreground to have nearly the same red color. In Tailwind v3 shadcn/ui, destructive-foreground was more white-ish, so was the shift to red intentional?

If error/etc text should use text-destructive instead of text-destructive-foreground, it makes sense to refactor those in this fix too.

Already seeing PRs addressing this mismatch: #6845.

@shadcn
Copy link
Collaborator

shadcn commented Mar 4, 2025

I think it's redundant for destructive and destructive-foreground

Agreed on this. We had a few a11y issues on v3 that I'm trying to address. There are other solutions I looked into but they are all breaking (i.e we'll need to introduce new variables).

I'm going to take a look at this again. Let me get back to you later today or tomorrow.

@shadcn shadcn self-assigned this Mar 4, 2025
@shadcn shadcn mentioned this pull request Mar 5, 2025
Closed
@Jacksonmills
Copy link
Contributor Author

LMK if I can be of help if there are any a11y issues you wanted to delegate

@shadcn shadcn added this to the v4 milestone Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@shadcn shadcn

Labels
None yet
Projects
None yet
Milestone
v4
Development

Successfully merging this pull request may close these issues.

[bug]: css var destructive-foreground is same color as destructive
2 participants
@Jacksonmills @shadcn