withastro / starlight

🌟 Build beautiful, accessible, high-performance documentation websites with Astro
https://starlight.astro.build
MIT License
5.17k stars 538 forks source link

[starlight-tailwind]: CSS warnings when setting a custom white tone using the tailwind plugin #1716

Closed Fryuni closed 7 months ago

Fryuni commented 7 months ago

What version of @astrojs/starlight-tailwind are you using?

2.0.1

What version of astro are you using?

4.5.16

What package manager are you using?

pnpm

What operating system are you using?

Linux

What browser are you using?

Chrome

Describe the Bug

When using the @astrojs/starlight-tailwind and configuring a custom white color, the build emits multiple warnings regarding CSS.

The config:

import starlightTailwind from '@astrojs/starlight-tailwind';
import colors from 'tailwindcss/colors';

/** @type {import('tailwindcss').Config} */
export default {
  content: ['./src/**/*.{astro,html,js,jsx,md,mdx,svelte,ts,tsx,vue}'],
  theme: {
    extend: {
      colors: {
        white: colors.stone,
      },
    },
  },
  plugins: [starlightTailwind()],
};

The warnings (truncated for brevity; it has about 20 of them):

21:52:50 [WARN] [vite] warnings when minifying css:
▲ [WARNING] Expected identifier but found "50" [css-syntax-error]

    <stdin>:14:4:
      14 │     50: #fafaf9;
         ╵     ~~

▲ [WARNING] Expected identifier but found "100" [css-syntax-error]

    <stdin>:15:4:
      15 │     100: #f5f5f4;
         ╵     ~~~

▲ [WARNING] Expected identifier but found "200" [css-syntax-error]

    <stdin>:16:4:
      16 │     200: #e7e5e4;

This problem only happens for the colors -> white field. I tested with colors -> accent and colors -> black; those fields don't have the same problem.

Passing other colors in that configuration also shows the same warning; it is not specific to Tailwind's Stone color.

The same configuration without the @astrojs/starlight-tailwind plugin shows no warnings.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-qea4d6?file=tailwind.config.mjs

Participation

delucis commented 7 months ago

Ah, interesting. I think this is because we use theme('colors.white') to get #fff in the Starlight Tailwind plugin, e.g.

https://github.com/withastro/starlight/blob/093a32ccbb962246b93180b6d4baa00cd8019025/packages/tailwind/index.ts#L68

The default Tailwind colors shape is something like this:

const colors = {
    transparent: "transparent",
    black: "#000",
    white: "#fff",
    gray: {
        50: "#f8fafc",
        100: "#f1f5f9",
        200: "#e2e8f0",
        // etc.
    },
    // other colors
}

So colors.white is expected to be a single string literal, not an object.

Usually when setting an alternate gray shade, this is done via gray: colors.stone. (This also makes sense logically because “white” and “black” are not categories that span a range of light to dark colours, but specific luminosities.)

Do you think we should support bending Tailwind’s palette naming or would a clear error for this case and not supporting it be acceptable?

Fryuni commented 7 months ago

But it works for black changing the pure black to a shade of the given color without any warnings, I think white should do the same, probably by taking the lightest shade of the color

delucis commented 7 months ago

But it works for black changing the pure black to a shade of the given color without any warnings, I think white should do the same, probably by taking the lightest shade of the color

Not really! Starlight’s colour scheme doesn’t use Tailwind’s black anywhere, it uses Gray 900 for its darkest shade. So if you set a different gray palette, it will use the 900 value from that palette. If you set a custom value for black in your Tailwind config, Starlight completely ignores it. But the white in Starlight is always 100% luminosity, i.e. #fff, so there is no variation between palettes.

If someone really, really wanted to have a different shade for their whites, they could set e.g. white: colors.stone.50. On the other hand, white: colors.stone is kind of a semantic “mistake” because it breaks Tailwind classes you might write: usually you can write text-white for example, but if you set the full palette, you’d have to use text-white-50 for the brightest shade, text-white would no longer be valid.

This kind of a conceptual distinction: there are palettes that range from dark to light (gray, indigo, cyan, etc.) and then there are individual named colours that don’t make sense with varying luminosity (white, black, transparent). If we take text-white-900 as an example (assuming white: colors.stone), then the resulting value is #1c1917. If you ask people what shade or colour #1c1917 is, “white” would be an unlikely answer, so that’s why usually you’d use gray: colors.stone and work with light and dark grays, not light and dark whites.

Fryuni commented 7 months ago

Interesting! In that case I think this should be an error at Tailwind level. Is this something I should report on Astro's Tailwind integration or on Tailwind directly? I have no idea how CSS tooling works and where each one's responsibility ends.

delucis commented 7 months ago

I think this should be an error at Tailwind level

I still think this is a Starlight-specific limitation that should have a nice error on our end rather than anything else. While Tailwind has that shape I described as a sensible default, it’s also in the end agnostic about what users decide to do with it. For example, Tailwind uses that <color>-<number> format for their shades, but it’s totally valid to provide a config shape that is completely different:

colors: {
  rainbow: {
    red: '#ff0000',
    orange: '#ffa500',
    yellow: '#ffff00',
    // ...
  },
},

And then use those as class="text-rainbow-red". Most people use Tailwind and only slightly tweak any of its defaults, but it also works as an entirely custom utility class generator (there’s an example a bit like this in this “A CSS project boilerplate” article).

So I don’t think Tailwind would see this as an error on their end, and the Astro integration does very close to nothing, so it’s not there either.

Instead I see it as:

If we were imposing a limitation that was unacceptable, we would aim to fix that and make our plugin more flexible. But in this case, I think expecting white to be a single colour, not a range of colours shouldn’t realistically cause problems for users. So instead we should make sure error messages and docs are more helpful on this point.