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

fix(module): stop using tailwind's shorthand arbitrary variable syntax #2366

Merged
merged 11 commits into from
Oct 14, 2024

Conversation

sandros94
Copy link
Collaborator

πŸ”— Linked issue

Resolves #2365

❓ 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

source: tailwindlabs/tailwindcss#13395 (comment)

[...] in v4, it will be max-w-[var(--breakpoint-sm)] as the shorthand arbitrary variable syntax will no longer be supported.

property-[--var-ref] ➞ property-[var(--var-ref)]

[...]

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@sandros94 sandros94 self-assigned this Oct 11, 2024
@benjamincanac benjamincanac linked an issue Oct 11, 2024 that may be closed by this pull request
Copy link

pkg-pr-new bot commented Oct 11, 2024

pnpm add https://pkg.pr.new/@nuxt/ui@2366

commit: 78b474a

Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

@sandros94 It seems you're missing some classes:

  • text-[--ui-text-muted]
  • text-[--ui-text-highlighted]
  • ring-[--ui-border-accented]
  • etc.

@sandros94
Copy link
Collaborator Author

sandros94 commented Oct 12, 2024

It seems you're missing some classes:

Yeah, I though about it as soon as I got into bed πŸ˜…

I'm fixing them up right now

@sandros94
Copy link
Collaborator Author

@benjamincanac all fixed up

Copy link
Member

I'm wondering if it would be a good idea to define all those as Tailwind CSS utilities.

Not exactly like before (https://github.com/nuxt/ui/pull/2298/files#diff-d57c48a7c2fad5e00150be2d599877390d949dd7409fb976099c0cd21a26971a) but something like this:

@theme default {
  --color-primary: var(--ui-primary);
  --color-secondary: var(--ui-secondary);
  --color-info: var(--ui-info);
  --color-success: var(--ui-success);
  --color-warning: var(--ui-warning);
  --color-error: var(--ui-error);

  --color-text-dimmed: var(--ui-text-dimmed);
  --color-text-muted: var(--ui-text-muted);
  --color-text-toned: var(--ui-text-toned);
  --color-text: var(--ui-text);
  --color-text-highlighted: var(--ui-text-highlighted);

  --color-bg: var(--ui-bg);
  --color-bg-elevated: var(--ui-bg-elevated);
  --color-bg-accented: var(--ui-bg-accented);
  --color-bg-inverted: var(--ui-bg-inverted);

  --color-border: var(--ui-border);
  --color-border-accented: var(--ui-border-accented);
  --color-border-inverted: var(--ui-border-inverted);
}

This would allow users to write text-primary / bg-primary / border-border / ...

We would keep the theme as-is with CSS variables so users choose to define the primary colors in their theme as described here it would be blocking.
CleanShot 2024-10-12 at 18.53.43@2x.png

What do you think?

Copy link
Member

benjamincanac commented Oct 12, 2024

(We cannot write this index.css file dynamically as we import it from @nuxt/ui otherwise it would require to import from #build/ui.css which is less .)

@sandros94
Copy link
Collaborator Author

What do you think?

Not an easy decision πŸ˜…

Personally speaking I do enjoy the idea of being in full control of the tailwind's theme within the end project.
But we are talking about utilities that are used by the vast majority of the projects, so ask myself: "why not?"

Just a thought: if we do add those utilities, does it mean that we do need to always include the related css variables?

@sandros94
Copy link
Collaborator Author

sandros94 commented Oct 13, 2024

Just an idea that I had:

Have multiple .css within the module. A default one that is used when doing @import "@nuxt/ui"; that does integrate the suggested @theme default, while also providing a @import "@nuxt/ui/minimal"; that mostly integrates the current index.css.

The module should be written and maintained with a css var approach (text-[var(--ui-color-primary)] instead of text-primary), making sure that both minimal and default work, but this approach would let module/layer authors to take full control, while also provide great out-of-the-box experience for non-advanced use cases.

P.S.: might also be useful to have a dedicated transition.css file

@benjamincanac benjamincanac merged commit dcce571 into nuxt:v3 Oct 14, 2024
2 checks passed
@benjamincanac
Copy link
Member

This is a good idea! I'll keep thinking about this 😊

@benjamincanac benjamincanac added the v3 #1289 label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 #1289
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsupported Tailwind's shorthand arbitrary variable syntax
2 participants