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

Do not force hsl and use tailwindcss theme colors whenever possible #185

Closed
wants to merge 1 commit into from

Conversation

emrosenf
Copy link
Contributor

@emrosenf emrosenf commented Apr 17, 2023

Incredible new update!

I re-organized your theming in a way that is more flexible and clear, especially for folks like myself who prefer to lean on the built-in tailwind.css palette.

Benefits

  • User is not forced to use HSL values
  • Currently, VSCode will not preview the color since it is not wrapped with hsl()
  • Makes it clear that you are still using the slate theme
  • Makes it simple to change the entire theme by doing a find-and-replace (e.g. slate -> stone)

Even if you don't push this, I think it could be added to the documentation as an alternative theme setup

@vercel
Copy link

vercel bot commented Apr 17, 2023

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

A member of the Team first needs to authorize it.

@jondot
Copy link

jondot commented Apr 18, 2023

exactly what i was trying to figure out. the numbers were too magical, but I assumed they all eventually come from slate. hope this merges in

EDIT: @emrosenf looks like hsl is needed for computations like hover:primary/90 otherwise it won't happen

@emrosenf
Copy link
Contributor Author

@jondot good catch. Looks like the Tailwind team is aware of the problem. That's a real shame.

Issue 1
Related PR, and permanent fix promised

@emrosenf
Copy link
Contributor Author

As a temporary solution, perhaps we could just add comments to globals.css. For example:

    /* theme(colors.white) */
    --background: 0 0% 100%;
    /* theme(colors.slate.900) */
    --foreground: 222.2 47.4% 11.2%;

Benefits:

  • Gives attribution to source
  • Less magical to end-user
  • Makes it easier to determine relative relationships if end-user wants to re-theme

With comments like this, if someone wanted to switch to stone, they could at least know to change the value representing slate.900 to the HSL equivalent of stone.900.

@shadcn would you be supportive of this?

@its-monotype
Copy link
Contributor

Make sense. It may not be immediately clear what colors are represented by these semantic values for those who are used to the built-in Tailwind palette, which could cause confusion. A great solution would be just to add comments in the docs for light theme, this would be great to understand what this color looks like in the Tailwind pallete. I think it would be enough. If a user wants to change colors they can replace them with their own HSL values. Probably there should be palette generators for dark colors, would be cool if anyone can provide a link for this.

Currently, VSCode will not preview the color since it is not wrapped with

That's true. But the name of the color should speak for itself, so I don't see any use case for it.

 --background: 0 0% 100%; /* white */
 --foreground: 222.2 47.4% 11.2%; /* slate-900 */

 --muted: 210 40% 96.1%; /* slate-100 */
 --muted-foreground: 215.4 16.3% 46.9%; /* slate-500 */

 --popover: 0 0% 100%; /* white */
 --popover-foreground: 222.2 47.4% 11.2%; /* slate-900 */

 --border: 214.3 31.8% 91.4%; /* slate-200 */
 --input: 214.3 31.8% 91.4%; /* slate-200 */

 --card: 0 0% 100%; /* white */
 --card-foreground: 222.2 47.4% 11.2%; /* slate-900 */

 --primary: 222.2 47.4% 11.2%; /* slate-900 */
 --primary-foreground: 210 40% 98%; /* slate-50 */

 --secondary: 210 40% 96.1%; /* slate-100 */
 --secondary-foreground: 222.2 47.4% 11.2%; /* slate-900 */

 --accent: 210 40% 96.1%; /* slate-100 */
 --accent-foreground: 222.2 47.4% 11.2%; /* slate-900 */

 --destructive: 0 100% 50%; /* red */
 --destructive-foreground: 210 40% 98%; /* slate-50 */

 --ring: 215 20.2% 65.1%; /* slate-400 */

@shadcn
Copy link
Collaborator

shadcn commented Apr 18, 2023

@emrosenf The color values follow the Tailwind CSS convention. https://tailwindcss.com/docs/customizing-colors#using-css-variables (as @jondot said this is for resolving alphas).

I like @its-monotype's inline comment suggestion to help improve documentation. Happy to merge a PR with this improvement.

Thank you

@emrosenf
Copy link
Contributor Author

Closing in favor of #200

@emrosenf emrosenf closed this Apr 18, 2023
@emrosenf emrosenf deleted the document-tailwind-theme branch April 18, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants