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

Proposal: make gray-* properly themable #2026

Closed
davestewart opened this issue Aug 7, 2024 · 8 comments · Fixed by #2298
Closed

Proposal: make gray-* properly themable #2026

davestewart opened this issue Aug 7, 2024 · 8 comments · Fixed by #2298
Labels
enhancement New feature or request v3 #1289 wontfix-v2 This will not be fixed in `v2.x`.

Comments

@davestewart
Copy link
Contributor

davestewart commented Aug 7, 2024

Description

TL;DR this is a proposal (with some prototyping done) to make gray theming more design system-friendly

Abstract

Right now, the gray-* CSS classes are reserved by Nuxt UI for borders, backgrounds, etc.

Unfortunately, due to how the theming works, the gray name cannot be changed, so developers have to use a word other than gray to reference real grays (as gray now effectively identifies "Nuxt UI's secondary color") such as cool:

<!-- simple grey box with themed text -->
<div class="border-cool-200">
  <div class="text-green-500">
    Some text
  </div>
</div>

My understanding is that this isn't going to change – I presume because it would be a breaking change for those already relying on gray-* in their apps.

Problem

Our existing Figma design system is based on borders and backgrounds using Tailwind's neutral color, so using Nuxt UI we're in a kind of "hybrid" situation where:

  • Figma uses neutral color variables
  • we specify neutral for Nuxt UI's theme
  • Nuxt UI components will render using gray-* classes (pointing at neutral variable values)
  • if we need gray elsewhere we need to use an alias, such as cool, slate, or currently a prefixed fgd-gray

Whilst I understand the decisions which got Nuxt UI here, and the problems that changing the naming convention would bring, I've been looking fairly deeply at the config and component code, and looking to see if there might be a way to give everyone what they want.

Proposal

Theming recap

As I understand it, Nuxt UI is styled is through ui.config files, and themed via {color} placeholders.

Several strategies combine to achieve the final HTML/CSS colorised output:

  • in the module
    • one of more colors are prepared to be safe-listed
    • a set of used component Tailwind properties is hardcoded
    • finally, these are combined to build a set of Tailwind safe-listed classname patterns and variants
  • in components

Expanding theming to better-support gray

So my conjecture is that:

  • we could use the same {placeholder} mechanism for the "gray" part of the theme (rather than only gray)
  • we safelist the configured primary and gray colors / classes
  • we replace both {color} and {gray} in components

I have done some initial prototyping, and it seems the theory is sound. It adds only a modest amount of safe-listed values (which in all likelihood would be created anyway) whilst removing the restrictions on what class names can be used.

The outcome is that Nuxt UI's component rendered markup would actually point to the Tailwind gray-color it is supposed to, leaving the user to use the actual "gray" class, without worrying that the "theme gray" class (i.e. neutral) would be different, without having to do the mental or physical gymnastics:

// app.config.js
{
  ui: {
    primary: 'teal',
    gray: 'neutral'
  }
}
// ui.config/elements/example.ts
export default {
  container: 'border-{gray}-200',
  content: 'text-{color}-500',
}
<!-- app (user can choose actual gray or actual neutral as required by the design) -->
<h1 class="bg-neutral-200 text-gray-500">My app</h1>

<!-- rendered example component (uses neutral, rather than the gray "alias") -->
<div class="border-neutral-200">
  <div class="text-teal-500">
    Some text
  </div>
</div>

Initial prototyping

I've experimented with the following code, with mainly good results.

Adding {gray} placeholders to ui.config/

Whilst I did this to make the RegExp replacements easier, I was surprised to see that when I imported the ui.config files, the {color} placeholders were already replaced with the configured primary value.

Clearly I don't understand the lifecycle of the module; perhaps you can connect the dots for me?

Making safelistByComponent() programmatic

I needed to test my idea was sound, so rather than manually add new options to safelistByComponent() I instead imported the ui.config objects and walked them to find all classes, then parsed and processed them to build the a unique set of Tailwind safelist configs.

This seem to work great!

As such, I was wondering why this wasn't done in the first place. I assume I'm missing something?

Improving ui.prop color replacement

Rather than replacing ui props on an individual level, I added a single helper would replace color and gray placeholders:

// from
const offIconClass = computed(() => {
  return twJoin(
    ui.value.icon.size[props.size],
    color.value && ui.value.icon.off.replaceAll('{color}', color.value)
  )
})
// to
import { applyColors } from '#ui/utils/colors'

const applyTheme = (value: string) => applyColors(value, props.color, appConfig.ui.gray)

const offIconClass = computed(() => {
  return applyTheme(twJoin(
    ui.value.icon.size[props.size],
    color.value && ui.value.icon.off
  ))
})

Again, this seems to work just fine.

Summing up

So, as mentioned, this is a kind of issue / proposal / sanity-check before doing any more work in this direction.

Maybe there are some fundamental issues that I haven't appreciated?

But my feeling is that we can make it work without impacting current users (perhaps even behind an experimental flag, like Nuxt Content, or such like). It would be really useful for us, and to others.

Some related tickets:

I'll be here to answer questions or follow up if I've overlooked things.

All the best,
Dave

Additional context

I can push a new branch to my Nuxt UI repo without a PR if you like, then you could review the changes there.

@davestewart davestewart added enhancement New feature or request triage labels Aug 7, 2024
@davestewart davestewart changed the title Proposal: make gray-* _properly_ themable Proposal: make gray-* properly themable Aug 7, 2024
Copy link
Member

@davestewart I read your issue quickly before leaving for vacation, I understand your point and before you start working on this I just want to say that I'm not sure it's a good idea to make such a refactor while v3 is on our doorstep. I think I'd rather focus on the next major version.

@davestewart
Copy link
Contributor Author

davestewart commented Aug 7, 2024

Thanks for taking the time to read it, and I understand your position.

So you know when the next version will be released? Will we need able to theme the "gray" component?

Maybe this will give you some ideas, at the very least!

Also, bonne vacances!

Copy link
Member

Have you checked https://github.com/benjamincanac/ui3?

@davestewart
Copy link
Contributor Author

Have you checked https://github.com/benjamincanac/ui3?

Not for a while, only for some UI prop related stuff.

You're developing this in isolation of the current version?

Copy link
Member

It's a fork, it's dev is pushed to the main repo from time to time: https://github.com/nuxt/ui/tree/v3 and will become the default branch once ready to publish a beta package!

Copy link
Member

To answer your previous question, for now it's quite basic and mimic the behaviour of the current version for the colors. The only difference is its config in ui.colors.primary and ui.colors.gray. We have a few limitation due to the CSS only config capabilities of Tailwind CSS v4 at the moment. But this version is a free canvas, we could totally find a way to improve colors 😊

Copy link
Member

Here are some additional informations about the v3 progress: #1289 (comment)

@davestewart
Copy link
Contributor Author

I see.

Ok, well, we can survive without this proposal, but in the meantime I'll check out the new repo. I'm sure it's 👌👌👌.

Cheers,
Dave

@benjamincanac benjamincanac added wontfix-v2 This will not be fixed in `v2.x`. v3 #1289 and removed triage labels Oct 6, 2024 — with Volta.net
@benjamincanac benjamincanac linked a pull request Oct 7, 2024 that will close this issue
8 tasks
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
Development

Successfully merging a pull request may close this issue.

2 participants