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

Using primary / gray as main colors might conflict with existing projects. #1721

Closed
codeflorist opened this issue Apr 26, 2024 · 9 comments · Fixed by #2298
Closed

Using primary / gray as main colors might conflict with existing projects. #1721

codeflorist opened this issue Apr 26, 2024 · 9 comments · Fixed by #2298
Labels
enhancement New feature or request v3 #1289 wontfix-v2 This will not be fixed in `v2.x`.

Comments

@codeflorist
Copy link

Description

primary, secondary, etc. is a rather popular naming scheme for tailwind-color-classes in many projects. This leads to problems when trying to add nuxt/ui to such a project, since it defines and uses primary itself. The same problem could arise with gray.

I would suggest renaming the classes to e.g. ui-primary, or make the class-name configurable.

Additional context

No response

@goela268
Copy link

goela268 commented May 3, 2024

I found the configuration ui does in the background very weird.

Turns out, you can not use primary and gray to name your colors in the tailwind.config.js as it will purge them out. However, if you name your color for example green (instead of primary) and cool (instead of gray), you can set this in the app.config.ts file:

export default defineAppConfig({
  ui: {
    primary: 'green',
    gray: 'cool'
  }
})

And then you are free to use bg-primary-500 again in your components and it will apply the correct color.

@benjamincanac
Copy link
Member

The goal of this feature is to be able to change your colors dynamically through your app.config.ts. The module creates Tailwind CSS primary and gray colors as aliases with CSS variables. This way we can use text-primary or text-gray across all the config and the user can choose which primary / gray they want in their app.config.ts instead of having to rewrite the entire config.

If you name a color primary or gray the module will erase it as we need the CSS variables.

I hear it might be a conflict in some projects but we're too committed to this naming to change it in my opinion. Even in the next major (v3), I don't plan to change this.

@codeflorist
Copy link
Author

@benjamincanac

Thanks for the explanation!

What would actually be detrimental from your side, if you used text-ui-primary instead of text-primary? What is the reason for your strict commitment to this, if i may ask?

From a workload-perspective, the change should be done pretty quickly using search and replace.

Imho this is a problem as old as CSS. It has been a long-time best practice for packages to have a specific prefix with their classes to not get into conflict with existing classes from the main project or other packages. Only now Tailwind is in the middle - but the problem is still the same.

What if other packages decide to also define their own primary colors?

@goela268
Copy link

I personally get the point, and don't really mind. However, I think the fact that primary and gray are reserved namespaces should be highlighted in a more obvious way instead of a text here: https://ui.nuxt.com/getting-started/theming#css-variables

I struggled for around one hour as to why tailwind purges out my own primary and gray color set, I thought the most obvious way to have a config that would be readable, would be to use primary and gray in my tailwind config and then declare those as the colors for Nuxt UI:

export default defineAppConfig({
  ui: {
    primary: "primary",
    gray: "gray",
  }
});

So If you don't intend in changing that in any way, I'd propose to use a more visible Alert for this on the theming page of the docs.

@davestewart
Copy link
Contributor

davestewart commented Aug 1, 2024

I just spent a few days scratching my head on this one, then a few hours experimenting on a large, mature project to try and get to the bottom of this!

It took a lot of detective work to finally work out why introducing primary variables from our Figma design system didn't work in our project, but they did work in the Nuxt Tailwind playground.

@benjamincanac – two options which could get round this; for any existing primary or gray config:

  1. use this config (rather than fetching the one indicated by ui.color) then overwrite it
  2. throw an error or warning in the build

Copy link
Member

So to summarize, we could solve this issue by adding a warning if we detect a primary color in the user's tailwind.config.ts? The gray color shouldn't be an issue as it's automatically renamed to cool.

@codeflorist
Copy link
Author

codeflorist commented Aug 2, 2024

So to summarize, we could solve this issue by adding a warning if we detect a primary color in the user's tailwind.config.ts? The gray color shouldn't be an issue as it's automatically renamed to cool.

It would point developers to the problem, but imho not solve the underlying problem.

Solving the problem in an affected project would require some manual refactoring, which can amount to many man-hours, probably turning off devs from using nuxt/ui at all.

nuxt/ui adding a prefix to it's own classes as proposed above would eliminate the problem from happening at all.

@davestewart
Copy link
Contributor

davestewart commented Aug 2, 2024

I'm actually about to submit a PR for this now.

It reuses any configured colors so should be a transparent fix, and would not require prefixing.

This might also fix the issue of "reserved" colors; @benjamincanac I'll push a WIP, then I'll see if we need any docs updates.

@davestewart
Copy link
Contributor

Interestingly, we've been caught out by a related problem in our project, in that we want to use neutral for "theme gray", but we also want to use gray for "actual gray", but with the current configuration, we can't have both.

However, I think I have a solution, but I'll need to test it, then PR it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v3 #1289 wontfix-v2 This will not be fixed in `v2.x`.
Projects
None yet
4 participants