jaredh159 / tailwind-react-native-classnames

simple, expressive API for tailwindcss + react-native
2.08k stars 84 forks source link

color key beginning with text only usable with text-text #273

Closed Ainias closed 8 months ago

Ainias commented 9 months ago

We have three different colors for text, which we named primary, secondary and tertiary. I've added them to our tailwind-config like so:


import colors from 'tailwindcss/colors';

export const tailwindConfig = {
    content: ['./src/**/*.{js,jsx,ts,tsx}'],
    theme: {
        extend: {
            colors: {
                text: {
                    primary: colors.slate[900],
                    secondary: colors.slate[500],
                    tertiary: colors.slate[400]
                },
                                 action: {
                    accent: colors.blue[600],
                    basic: colors.slate[600]
                },
            }
        }
    }
};

But with tw.color('text-primary') it is not working. I have to use tw.color('text-text-primary'). Different keys like tw.color('action-accent') are working without any problems

jaredh159 commented 9 months ago

hmmm... i'm not sure this is really a bug. you have a color that is called text-primary, which means that if you wanted to set your text to be that color, the utility you'd need to use is text-text-primary, which is a little weird looking.

the tw.color() fn just lets you pass some utility that includes a color, and strips out the prefix. so, for instance, it allows text-text-primary but also bg-text-primary or border-text-primary. in all three cases, it strips out the prefix. so what's happening here is that when you pass it text-primary it thinks the text- part is indicating this is a text color utility (not a bare color), so it's stripping it. you could pass bg-text-primary if that felt better.

but i guess, my thought is that this is more of a side-effect of an unusual color name with a straightforward workaround, so i'm thinking it doesn't make sense to make the function more intelligent.

does that reasoning make sense and seem right to you?

Ainias commented 9 months ago

I understand your reasoning, but I would still argue that this as a bug.

Nothing in the docs says you can pass any tailwind-class to the tw.color-method. In fact it just says that you can insert any tailwind-color, which text-primary is according to my config. I would assume that tw.color does not alter the class and cannot be used with any tailwind-class. If you want to allow all tailwind-classes, it should be represented inside the docs and also mentioned, that the prefixes will be stripped, if it is text, bg or similar.

PS we also have the following inside our config:

textColor: {
                primary: colors.slate[900],
                secondary: colors.slate[500],
                tertiary: colors.slate[400],
                disabled: colors.slate[300]
            },          

This allows us to use the text-primary classes directly to set the text-color like <Text style={tw.style("text-prmary")}>My Text</Text>. We do not want to add the colors directly (without the text-prefix) to the color key, as then bg-primary would be a valid class. But this should have another color, so we use also the backgroundColor key inside the tailwind-config for that.

jaredh159 commented 9 months ago

i'm sympathetic to your thought process here. maybe i'm wrong. possibly the implementation of the color func could naively attempt the current method, and then fall back to actually inspecting the config (being careful not to emit warnings in dev mode, ideally).

my biggest concern would be performance though -- if i felt like i couldn't implement it in a way that would be fast, i think i would prefer to change the documentation and formalize the current behavior. but i sort of doubt that will be the case.

going to think on it a bit, and maybe test a change next time i carve out a little time to work on this library.

Ainias commented 9 months ago

I understand your argument and a config change is also fine :)

Let me know how you decide. If you do not change the current behaviour, I'll write a simple function for our project that only uses the config, as this is exactly what we need.

jaredh159 commented 8 months ago

available in v4.0.1. thanks for the issue 👍