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(module): reuse existing primary and gray colors #2009

Closed
wants to merge 8 commits into from

Conversation

davestewart
Copy link
Contributor

@davestewart davestewart commented Aug 2, 2024

πŸ”— Linked issue

Resolves #1721

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Right now, the color names primary and gray are reserved by Nuxt UI so it can build alpha-ready versions of the primary colors used in the individual components.

The problem with this is that it overwrites existing colors, and means users can't use the name "primary" in their themes (and possibly design-systems if using codegens).

This PR attempts to fix this by first detecting if the user has used a key called primary (and gray) and then copies these values to $primary (and $gray) aliases.

The existing machinery then checks if the user's ui config points to primary and gray, then in the Nuxt plugin property, points towards the aliased / backed-up values instead, to generate the CSS variables.

I've checked it out in the playground and it seems to work well, but suggest a check on site in the wild!

      <div>
        <p class="mb-4 p-2 leading-none
                  text-gray-500 hover:text-gray-900
                  bg-primary-500 hover:bg-primary-200
                  ">Click below</p>
        <UButton>I should be themed</UButton>
      </div>

I can do this with our current site.

Also, looking at any docs updates now.

πŸ“ Checklist

@davestewart davestewart marked this pull request as draft August 2, 2024 11:21
@davestewart
Copy link
Contributor Author

davestewart commented Aug 2, 2024

@benjamincanac – right now, Nuxt UI renames any gray variables it finds to cool but in theory, this fix means that won't be needed in future – as the colors will just be converted to their variables equivalent (but I guess it should be kept in for backwards compatibility / existing projects).

I wanted to check though, in case I missing some hidden situations?

As such, I think the CSS Variables section of the theming docs could be simplified.

I propose:

  • clarifying what primary and gray do in the Configuration section
  • rename the CSS Variables section to "How it works"
  • remove the warnings about primary and gray values
  • explain instead how the raw colours are converted to rgba placeholder values so Tailwind can apply alpha values

What do you think?

@benjamincanac benjamincanac changed the title feat(Theming): reuse existing primary and gray colors feat(module): reuse existing primary and gray colors Aug 2, 2024
* dev:
  fix(module): handle nested colors from ui config (nuxt#2008)
@benjamincanac
Copy link
Member

@davestewart Do you think it would be a breaking change to remove the cool color? It was there mostly to be a proxy for the gray (CSS variable) color.

I 100% agree with your proposal about the docs, will review in other PR 😊

@davestewart
Copy link
Contributor Author

Do you think it would be a breaking change to remove the cool color

I was hoping you would know!!! Or at least, be more familiar with the logic / situations.

I guess it would be simple to check in the playground, or even spin up a dummy project.

@benjamincanac
Copy link
Member

Honestly, it kind of depends if we can achieve the same thing on v3. I don't want to remove it now to reintroduce the cool color in next major πŸ˜… We're kind of limited at the moment since Tailwind CSS v4 is still in alpha but here is how it's done for now: https://github.com/benjamincanac/ui3/blob/dev/src/templates.ts#L23-L34

@davestewart
Copy link
Contributor Author

davestewart commented Aug 2, 2024

OK, I think we're good!

I have left in the default to cool so the behaviour before and after should be identical.

Setup

I tested app.config.ts with various ui.gray settings:

undefined
'slate'
'gray' // no tw config
'gray' // custom tw config

I used the following playground code:

<template>
  <UContainer class="min-h-screen max-w-sm !mx-auto flex flex-col justify-center content-center gap-8 m-8">
    <UCard>
      <template #header>
        Gray
      </template>

      <p class="p-2 leading-none
                text-gray-900 hover:text-gray-900
                bg-gray-600 hover:bg-gray-500
                ">bg-gray-600 (themed as {{ ui.gray }})</p>
      <p class="p-2 leading-none
                text-slate-900 hover:text-slate-900
                bg-slate-600 hover:bg-slate-500
                ">bg-slate-600</p>
      <p class="p-2 leading-none
                text-cool-900 hover:text-cool-900
                bg-cool-600 hover:bg-cool-500
                ">bg-cool-600</p>
    </UCard>
  </UContainer>
</template>

<script setup>
const ui = useAppConfig().ui
</script>

I (optionally) used the following gray config (just so it is obvious):

CleanShot 2024-08-02 at 21 50 51

        gray: {
          '50': '#edfffc',
          '100': '#c0fff9',
          '200': '#81fff4',
          '300': '#3affef',
          '400': '#00ffe1',
          '500': '#00e2c9',
          '600': '#00b7a6',
          '700': '#009185',
          '800': '#00726b',
          '900': '#045d58',
          '950': '#003a39',
        },

Results

I got the following results:

  • undefined // defaults to 'cool':

    image

  • slate:

    image

  • gray // no tw config, same as cool:

    image

  • gray // custom tw config, custom colors:

    image

Conclusion

So I think that means:

Primary:

  • only if the user supplies a primary config and specifies ui.primary as primary will they get their custom primary
  • otherwise, they get a default Tailwind color from ui.primary or Nuxt UI's default green

Gray:

  • only if the user supplies a gray config, and specifies ui.gray as gray will they get their custom gray
  • otherwise, they get Tailwind gray aka cool

It also seems to mean that you could change the default ui.gray to be gray and cool would still be configured in the background, so for anyone using cool, it would still work (as in, show as gray):

image

As I understand it, the only reason to leave it would be if it confused some people if it was changed, but AFAIK gray and cool are the same thing. But, totally up to you of course (probably best to keep the status quo!).

* dev:
  fix(module): reduce css bundle size by fixing safelist regex (nuxt#2005)
  fix(useFormGroup): app config default input size (nuxt#2011)
@benjamincanac
Copy link
Member

@davestewart I'll review and merge your PRs tomorrow after a patch release including #2008 😊

@davestewart
Copy link
Contributor Author

Enjoy the rest of your weekend πŸ™Œ

return excludeColors(globalColors)
return [
...excludeColors(globalColors),
...Object.keys(userColors)
Copy link
Member

Choose a reason for hiding this comment

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

You should not export $gray or $primary here, otherwise it gets exported from #ui-colors (https://github.com/nuxt/ui/blob/dev/src/templates.ts#L4) and will be prompted to users in color prop: https://github.com/nuxt/ui/blob/dev/src/runtime/types/button.d.ts#L8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Taking a look now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure of the best way to handle this, or sufficiently confident I understand some of the subtleties of the code.

A simple option might be to filter the keys in the template creation function, but I am wondering if perhaps I misunderstood the role of setGlobalColors() and perhaps the theme object should not be augmented here.

If that was the case, then maybe in src/tailwind.ts we should detect and add primary and gray to nuxt.options.appConfig.ui.themeColors and refactor how plugins/colors.ts determines where to pull the colors from, referencing ui.themeColors instead.

I'll try this second option now, but if you want to offer any advice, I'm happy to hear it.

Copy link
Contributor Author

@davestewart davestewart Aug 5, 2024

Choose a reason for hiding this comment

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

I'm guessing, looking at the interface for app config, that perhaps app config is the wrong place to temporarily save these colors.

The challenge is caching primary and gray somewhere so they can be accessed in plugins/colors.ts as by this point they will have been overwritten in the theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, think I have it using runtimeConfig.

It's a bit late now, but I'll submit a revised PR tomorrow.

@davestewart
Copy link
Contributor Author

davestewart commented Aug 7, 2024

Good morning @benjamincanac β˜€οΈ

I've moved the logic of separating user vs global colors from setGlobalColors() to installTailwind() and am transferring the color values to the Nuxt plugin via runtimeConfig.

I have some Nuxt module experience, but usually just what I need to learn to get the job done; perhaps you can let me know if that is the right place.

@davestewart
Copy link
Contributor Author

Just checking in Benjamin. Hope your holiday was good.

@benjamincanac
Copy link
Member

Hey @davestewart, it was good thank you :)

I'm still not sure what to do about this regarding the future compatibility with v3, I don't want to introduce this feature to go back to the old system in the next major it's confusing 😬

However, what do you think of merging #2010 and #2014 first? I think only a few changes needs to be done in #2010.

@davestewart
Copy link
Contributor Author

davestewart commented Sep 7, 2024

Morning!

I'm still not sure what to do about this regarding the future compatibility with v3, I don't want to introduce this feature to go back to the old system in the next major it's confusing 😬

I understand your concerns about going back.

Remember, the core idea is you can use any theme color names you like, without having to work around reserved names.

If the implementation is transparent, should it not be possible in either version?

The code in this version isn't particularly complex, it's just thinking about the problem slightly differently.

However, what do you think of merging #2010

Sure! Let me take a look at see if what I wrote still works.

and #2014 first

Once #2010 is merged, I can update the docs

Haven't looked at this stuff for six weeks or so, so need to re-familiarise myself :)

@benjamincanac
Copy link
Member

@davestewart I'm thinking of closing this and keep it as-is for v2. I've taken an entirely different approach that fixes #1721 in v3, you can check out #2298.

@davestewart
Copy link
Contributor Author

Hey @benjamincanac, sure, I get it. It looks like a lot of thought has gone into your new system. I'm away right now so will just say do what you gotta do!

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.

Using primary / gray as main colors might conflict with existing projects.
2 participants