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

Wrong merge of classes with text-prefix #368

Closed
christophstockinger opened this issue Jan 11, 2024 · 17 comments
Closed

Wrong merge of classes with text-prefix #368

christophstockinger opened this issue Jan 11, 2024 · 17 comments
Labels
context-v2 Related to tailwind-merge v2

Comments

@christophstockinger
Copy link

Describe the bug

I have the following definition in my Tailwind-Config:

/** @type {import('tailwindcss').Config} */

const defaultTheme = require('tailwindcss/defaultTheme')

module.exports = {
  theme: {
    fontSize: {
      ...
      // Button
      button: [
        'clamp(1.5rem, 1.2rem + 1.13vw, 2rem)',
        {
          lineHeight: '1',
          letterSpacing:
            'clamp(0.015rem, 0.01rem + 0.05vw, 0.04rem)'
        }
      ],
      ...
    },
    ...
  }
}

If I now define the classes text-button and text-black on a button, the class text-button is removed.

However, this should not happen because the two classes have different instructions. The class text-button only takes care of the font-size and is a pardon to text-base. The class text-black is responsible for the color.

To Reproduce

Tailwindconfig as above

// cn.ts
import clsx, { ClassValue } from 'clsx'
import { twMerge } from 'tailwind-merge'

export const cn = (...inputs: ClassValue[]) => {
  return twMerge(clsx(inputs))
}

```tsx
// button.tsx
import cn from './cn'

const Button = () => {
  const baseStyle = 'font-sans text-button font-extrabold border-6'

  const primaryStyle = 'border-black bg-white text-black'

  return (<button className={cn(baseStyle, primaryStyle)}>Label</button>
}

Expected behavior

Both classes are rendered and the corresponding value is used for merging.

Environment

  • node version: v20.6.1
  • next.js version: 13.4.7
  • clsx version: 1.2.1
  • tailwindcss version: 3.3.2
  • tailwind-merge version: 2.2.0

Additional context

./.

@dcastil
Copy link
Owner

dcastil commented Jan 11, 2024

Hey @christophstockinger! 👋

tailwind-merge doesn't have access to the tailwind.config.js file and you need to configure it separately so it knows about the text-button class.

Here is an example on how to configure tailwind-merge: https://github.com/dcastil/tailwind-merge/blob/v2.2.0/docs/recipes.md#adding-custom-scale-from-tailwind-config-to-tailwind-merge-config.

And here is the documentation on how the tailwind-merge configuration works: https://github.com/dcastil/tailwind-merge/blob/v2.2.0/docs/configuration.md#usage-with-custom-tailwind-config.


(For myself)

Related: #322, #321, #315, #302, #276, #275, #274, #250, #207

@dcastil dcastil added the context-v2 Related to tailwind-merge v2 label Jan 11, 2024
@christophstockinger
Copy link
Author

Hi @dcastil ,

sorry, but I don't see how I can get both classes out at the end. Unfortunately, the documentation is not really helpful here. You explained an example with box-shadow because there is a special case. But a concrete example with initial situation, configuration and result would be more helpful.

@dcastil
Copy link
Owner

dcastil commented Jan 13, 2024

This is the setup code you need.

import { extendTailwindMerge } from 'tailwind-merge'

const twMerge = extendTailwindMerge({
    // use the `extend` key in case you want to extend instead of override
    override: {
        classGroups: {
            'font-size': ['text-button', ]
        }
    }
})

Then you use the twMerge function from the code above everywhere instead of the one exported from tailwind-merge.

Not sure what special case you mean with box-shadow. The procedure should be the same like for font-size classes.

@ViktorPontinen
Copy link

ViktorPontinen commented Jan 18, 2024

Hi, I have same issue. I have defined a lot of extra classes using extendTailwindMerge, maybe I missed something somewhere and you could give a pointer?

My twMerge wrapper function:

import cx from 'classnames'
import type classNames from 'classnames'
import { extendTailwindMerge } from 'tailwind-merge'

import { colors } from '../../tailwind-config/colors'
import { surface } from '../../tailwind-config/semantics/surface'
import { border } from '../../tailwind-config/semantics/border'
import { background } from '../../tailwind-config/semantics/background'
import { touchHeight } from '../../tailwind-config/semantics/touchHeight'
import { fontFamilyGroup } from '../../tailwind-config/fonts/fontFamily'
import { translucent } from '../../tailwind-config/semantics/translucent'
import { borderRadiusKeys } from '../../tailwind-config/semantics/borderRadius'
import { paddingKeys } from '../../tailwind-config/semantics/padding'
import { marginKeys } from '../../tailwind-config/semantics/margin'
import { fontSize } from '../../tailwind-config/semantics/fontSize'
import { textColor } from '../../tailwind-config/semantics/textColor'

const twMerge = extendTailwindMerge({
  extend: {
    theme: {
      spacing: Object.keys(touchHeight).map((key) => key),
      colors: [
        'transparent',
        'currentColor',
        ...Object.keys(colors).map((key) => key),
        {
          surface: Object.keys(surface).map((key) => key),
        },
        {
          border: Object.keys(border).map((key) => key),
        },
        {
          background: Object.keys(background).map((key) => key),
        },
        {
          text: Object.keys(textColor).map((key) => key),
        },
        {
          translucent: Object.keys(translucent).map((key) => key),
        },
      ],
      borderRadius: Object.keys(borderRadiusKeys).map((key) => key),
      padding: Object.keys(paddingKeys).map((key) => key),
      margin: Object.keys(marginKeys).map((key) => key),
    },
    classGroups: {
      'font-size': Object.keys(fontSize).map((key) => key),
      'font-family': Object.keys(fontFamilyGroup).map((key) => key),
    },
  },
})

export const cn = (...inputs: classNames.ArgumentArray) => {
  console.log('Inputs:', inputs)
  const result = twMerge(cx(inputs))
  console.log('Outputs: ', result)
  return result
}

Inputs console log:
inline-flex items-center justify-center whitespace-nowrap transition-colors disabled:pointer-events-none focus:ring-2 ring-inset focus:ring-border-stroke-focused bg-background-stone-darker bg-surface-primary text-text-light hover:bg-surface-primary-hover focus:bg-surface-primary-focused active:bg-surface-primary-selected disabled:bg-surface-primary-disabled disabled:text-text-pale h-touch-height-lg px-lg text-content-lg font-semibold rounded-md

Output console log:
inline-flex items-center justify-center whitespace-nowrap transition-colors disabled:pointer-events-none focus:ring-2 ring-inset focus:ring-border-stroke-focused bg-surface-primary hover:bg-surface-primary-hover focus:bg-surface-primary-focused active:bg-surface-primary-selected disabled:bg-surface-primary-disabled disabled:text-text-pale h-touch-height-lg px-lg text-content-lg font-semibold rounded-md

Note the text-text-light class is missing in the output (yes, naming of that class is probably horrible, but that is besides the point, I've tried using different "prefix" instead of "text" and it is still removed).

Interesting tho that disabled:text-text-pale is not removed, even if I add another disabled:text-text-light before it, it is still resolved properly.

From what I can tell it seems to think that text-content-lg and text-text-light are conflicting.

@ViktorPontinen
Copy link

ViktorPontinen commented Jan 18, 2024

Hi, I have same issue. I have defined a lot of extra classes using extendTailwindMerge, maybe I missed something somewhere and you could give a pointer?

...

Please ignore this. It was a config issue.

Changed

    classGroups: {
      'font-size': Object.keys(fontSize).map((key) => key),
      'font-family': Object.keys(fontFamilyGroup).map((key) => key),
    },

to

    classGroups: {
      'font-size': Object.keys(fontSize).map((key) => `text-${key}`),
      'font-family': Object.keys(fontFamilyGroup).map((key) => `font-${key}`),
    },

and it worked! :)

@dcastil
Copy link
Owner

dcastil commented Jan 19, 2024

@ViktorPontinen nice that you could figure it out! In the classGroups object the config needs the full class names indeed. I can see how that can be confusing when also using the theme object. 🤔

@WesselKroos
Copy link

WesselKroos commented Jan 22, 2024

@dcastil We are having the same issue with our custom font-size and colors. Configuring the classGroups like @ViktorPontinen fixed it mostly.

But currently we are still having overlapping classes when we use the line-height modifier on the font-size class text-<font-size>/<line-height>:

twMerge('text-10/100', 'text-12/120') becomes text-12/120
twMerge('text-10 leading-100', 'text-12/120') becomes text-12/120
twMerge('text-10 leading-100', 'leading-120') becomes text-10 leading-120
twMerge('text-10/100', 'leading-120') becomes text-10/100 leading-120
twMerge('text-10 leading-100', 'text-12') becomes text-12
twMerge('text-10/100', 'text-12') becomes text-12

Is there a configuration option that could fix this?
If not, I could imagine that a possible solution/workaround would be to support those by automatically splitting a modified font-size to text-<font-size> and leading-<line-height> before twMerge merges these classes together?

@dcastil
Copy link
Owner

dcastil commented Jan 22, 2024

Hey @WesselKroos! 👋

The bottom three cases are intended and should be correct (at least in the default config). Here is an explanation.

twMerge('text-10/100', 'leading-120') === 'text-10/100 leading-120'

Because leading-120 is defined after text-10/100, it is treated as a refinement, meaning that you want to apply the font-size of text-10/100 and the line-height of leading-120. tailwind-merge needs to keep both classes in there because you want to apply properties from both and tailwind-merge lets the CSS cascade do the merging.

tailwind-merge also can't remove the /100 part from text-10/100 because that class might not actually be in the final CSS if you aren't using it anywhere else. E.g. if you create a codebase with only the class text-10/100, then Tailwind CSS will only output that single class into the final CSS (plus the base stuff) and text-10 will not be in it.

You can read more on refinements in #156 (comment).

twMerge('text-10 leading-100', 'text-12') === 'text-12'

Here the leading-100 class gets removed because in the default config, the font-size classes also set the line-height property, so tailwind-merge assumes you want to override the line-height as well. More info on that in #59 (comment).

If you changed your Tailwind CSS config so that font-size classes don't set a line-height you can tell tailwind-merge that font-size classes aren't in conflict with line-height classes:

const twMerge = extendTailwindMerge({
    override: {
        conflictingClassGroups: {
            // The original config is here: https://github.com/dcastil/tailwind-merge/blob/v2.2.0/src/lib/default-config.ts#L1778
            'font-size': [],
        }
    }
})

twMerge('text-10/100', 'text-12') === 'text-12'

This one is a bit more complicated. text-10/100 is removed because another font-size class comes afterwards. In the default config the font-size class sets a line-height, so it makes sense to remove preceding font-size classes, even if they have a line-height modifier.

But if you'd remove the conflict from the config like in the example above, it would still get removed here because the first part text-10 is still in conflict with text-12. I think there is no good solution here, because if I keep the class in, the font-size of text-10 might get applied although text-12 comes afterwards and if I remove it, the /100 part is lost. Keep in mind that tailwind-merge can only remove entire classes from a string, it can't modify a class because of how the Tailwind CSS compiler works.

@WesselKroos
Copy link

WesselKroos commented Jan 22, 2024

Hi @dcastil 👋
Thanks for the detailed explanation.

Your workaround for the second case was what I was still looking for. That has been resolved now that I've removed the font-size from the conflictingClassGroups.

Regarding the first and third cases, I was able to resolve those by splitting up a combined font-size/line-height via a regex:

// Replaces 'text-12/100' with 'text-12 leading-100' to prevent 'leading-110' from keeping 'text-12/100' and conflicting
const splitFontSizeAndLineHeight = (className?: ClassNameValue) =>
  typeof className === 'string'
    ? className.replace(
        /text-([0-9]+)\/([0-9]+)/g,
        (_groups, text, leading) => `text-${text} leading-${leading}`,
      )
    : className;

export const cn = (...classNames: ClassNameValue[]) =>
  twMerge(
    ...classNames.map((className) => splitFontSizeAndLineHeight(className)),
  );

So all issues in our project have been resolved. Thanks once again!

@dcastil
Copy link
Owner

dcastil commented Jan 22, 2024

@WesselKroos nice workaround! Just keep in mind that the transformation is unsafe and that you could end up in a case where the Tailwind style won't get applied because the class is missing in CSS. More on that in https://tailwindcss.com/docs/content-configuration#dynamic-class-names.

@WesselKroos
Copy link

WesselKroos commented Jan 22, 2024

@dcastil Thanks for the extra tip. I think I should be able to workaround that by safelisting our font-sizes and line-heights.

@sabvente
Copy link

sabvente commented Mar 6, 2024

Hi @dcastil,

I'm writing as my last resolt, as I tried many configurations with extend/override, classGroups, conflictingClassGroups, etc...

TW custom font size config:

{
  theme: {
    extend: {
      fontSize: {
        'dr': ['72px', {lineHeight: '72px', letterSpacing: '-0.01em', fontWeight: 400}]
      }
    }
  }
}

Following your comment this is my config extension:

const customTwMerge = extendTailwindMerge({
  override: {
    classGroups: {
      'font-size': ["text-dr"],
      'leading': ["text-dr"],
      'tracking': ["text-dr"],
      'font-weight': ["text-dr"]
    }
  }
});
console.log(customTwMerge("text-5xl", "leading-5", "font-light", "tracking-tighter", "text-dr"));

The result is text-5xl leading-5 font-light tracking-tighter text-dr. However, I expect to get text-dr only.

Use case 2:
Is there a way to create a config to achieve
text-5xl text-dr text-3xl => text-dr text-3xl?

tailwind-merge 2.2.1

@dcastil
Copy link
Owner

dcastil commented Mar 9, 2024

Hey @sabvente! 👋

Here is the config you need with some explaining comments.

const twMerge = extendTailwindMerge({
    // We're using `extend` because we want to add a value and not override an entire group
    extend: {
        classGroups: {
            // Adding the custom font-size class here
            'font-size': ['text-dr'],
        },
        conflictingClassGroups: {
            // Adding the info that font-size conflicts with font-weight and tracking.
            // Keep in mind that this makes all font-size classes conflict with font-weight tracking.
            // We don't need to define leading here because that is already in the default config.
            'font-size': ['font-weight', 'tracking'],
        },
    },
})

For your use case 2: Not sure why you'd want to do this but you'd need to create a new group for text-dr which conflicts with the usual font-sizes but not the other way around. You can do that with this config where I call this extra group font-size-2:

// In case you're using TypeScript: We need to tell tailwind-merge that we want to use a new class.
const twMerge = extendTailwindMerge<'font-size-2'>({
    // We're using `extend` because we want to add a value and not override an entire group
    extend: {
        classGroups: {
            // Adding the custom group with your font-size class here
            'font-size-2': ['text-dr'],
        },
        conflictingClassGroups: {
            // Adding the info that our font-size-2 group conflicts with font-size, leading, font-weight and tracking.
            // Keep in mind that font-size won't conflict with font-size-2 as you want it.
            // For that you'd need to add `'font-size': ['font-size-2']` here.
            'font-size-2': ['font-size', 'leading', 'font-weight', 'tracking'],
        },
    },
})

@dcastil
Copy link
Owner

dcastil commented Mar 9, 2024

In case you have more questions, please open a new issue. No need to send notifications to all the people in this thread. 😊

I'm closing this one as resolved.

@dcastil dcastil closed this as completed Mar 9, 2024
@JRebella
Copy link

tailwind-merge doesn't have access to the tailwind.config.js file

Hey @dcastil this might sound dumb but why isn't this the case? Shouldn't the full tailwind.config file be enough to automatically figure out all the conflicting classes and avoid having to configure twMerge as well?

In fact, the Tailwind CSS IntelliSense VS code plugin probably does this already since it manages to warn you when you have conflicting classes in the same string and it automatically picks up custom configurations from the config file

@WesselKroos
Copy link

@JRebella tailwind runs at build-time, while tailwind-merge runs at runtime. So, including the full tailwind.config in tailwind-merge means that the final bundle size increases a lot. There is a request for tailwind to make it possible for third party tools to retrieve information about all final used classes (like a config which only contains the used configurations).

You can get more info in this discussion: #413 (comment)

@dcastil
Copy link
Owner

dcastil commented Jun 28, 2024

Hey @JRebella! 👋

@WesselKroos already said it all. It is possible to create a Tailwind plugin that runs at build-time. But it's difficult to get the ergonomics right because the plugin would need to generate code which can easily create a frustrating experience. Especially during development with hot module replacements, etc.

tailwind-merge is just the runtime part of it, but anyone can build on top of it and expand it to generate the config automatically. Maybe I'll do it some day, when I have some time and energy for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context-v2 Related to tailwind-merge v2
Projects
None yet
Development

No branches or pull requests

6 participants