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

background-image and background-position classes are being removed wrongly #297

Closed
iduuck opened this issue Aug 22, 2023 · 21 comments · Fixed by #300
Closed

background-image and background-position classes are being removed wrongly #297

iduuck opened this issue Aug 22, 2023 · 21 comments · Fixed by #300
Labels
bug Something isn't working context-v2 Related to tailwind-merge v2
Milestone

Comments

@iduuck
Copy link

iduuck commented Aug 22, 2023

Describe the bug

When supplying multiple "similar" classes, some are removed wrongly.

To Reproduce

https://codesandbox.io/s/magical-sound-6sfws8?file=/src/index.ts

Expected behavior

Background size, and image should be included.

Environment

  • tailwind-merge version: 1.13.2

Additional context

N/A

@dcastil dcastil added the bug Something isn't working label Aug 22, 2023
@dcastil dcastil added this to the v2 milestone Aug 22, 2023
@dcastil
Copy link
Owner

dcastil commented Aug 22, 2023

Hi @iduuck! 👋

That's a bug indeed, thanks for reporting! I'll fix that.

I'll include this in a v2 release I'm working on, so I won't be able to ship the fix immediately.

As a temporary workaround you can use the tag url: at the beginning of the arbitrary value for background-images (even if they aren't actually URLs) like this:

bg-[url:linear-gradient(-45deg,#fff_0%,#f9fafb_100%)]

Tailwind CSS accepts that value and tailwind-merge will detect it as a background-image class.

@dcastil
Copy link
Owner

dcastil commented Aug 22, 2023

Note to myself:

@dcastil
Copy link
Owner

dcastil commented Aug 22, 2023

@iduuck I just realized that arbitrary values in background-position and background-size classes are currently only supported by using tags position: and size:, otherwise they'll get classified as background colors and merged with other background colors. So if you're using arbitrary values in those, you need to keep that in mind.

I'll either fix this as well or make this clear in the documentation.

@locbqvmo
Copy link

@dcastil Having similar issue here, when applying classNames like text-red-500 and text-lg, one of them will be removed because having the same prefix

@dcastil
Copy link
Owner

dcastil commented Aug 24, 2023

Hey @locbqvmo! 👋 Could you open a separate issue for that and provide a reproduction example? I just played around on https://codesandbox.io/s/tailwind-merge-playground-cssr35?file=/src/index.ts with

const mergeData: MergeData[] = [
    {
        args: ['text-red-500 text-lg'],
    },
]

and it outputs text-red-500 text-lg which seems correct.

@dcastil dcastil changed the title Similar classes are being removed wrongly (e.g. background-image and background-position) background-image and background-position classes are being removed wrongly Aug 24, 2023
@ankhoa18599
Copy link

ankhoa18599 commented Aug 29, 2023

Hi @dcastil. Try with a custom tailwind config. I define new fontSize: "40px".

//tailwind.config.js
module.exports = {
  ...
  theme: {
    extend: {
      fontSize: {
        "40px": ["40px", "56px"],
      },
    },
  },
  ...
};

My component pass props

className={twMerge("mb-10 text-center text-40px font-semibold text-bbc-brand-800 ")}

and I got result:

image

The text-40px is in conflict with text-bbc-brand-800 so text-40px class are being removed wrongly.

But, if I try with name is "text-40xl":

//tailwind.config.js
module.exports = {
  ...
  theme: {
    extend: {
      fontSize: {
        "40xl": ["40px", "56px"],
      },
    },
  },
  ...
};
className={twMerge("mb-10 text-center text-40xl font-semibold text-bbc-brand-800 ")}

I got true result:
image

The text-40xl isn't in conflict with text-bbc-brand-800.

The same issue if I try text-40size, text-40abc,... Only text-40xl same suffix "xl" with text-xl, text-2xl,.. of tailwind default return true.

@dcastil
Copy link
Owner

dcastil commented Aug 29, 2023

Hey @ankhoa18599! 👋

tailwind-merge doesn't have access to the tailwind.config.js file and you need to configure it separately. The default out-of-the-box config of tailwind-merge assumes all unknown classes which could be a color are a color, this is why it de-duplicated text-40px when you use text-bbc-brand-800 afterwards.

Here is an example on how to configure tailwind-merge: https://github.com/dcastil/tailwind-merge/blob/v1.14.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/v1.14.0/docs/configuration.md#usage-with-custom-tailwind-config.

@Senneer
Copy link

Senneer commented Aug 29, 2023

Hi. I have the same issue as @ankhoa18599.

@dcastil could you expand on how one can configure extendTailwindMerge? There is no fontSize config inside theme (considering https://github.com/dcastil/tailwind-merge/blob/v1.14.0/docs/configuration.md#theme).

Even though I added all my custom color names inside theme.colors, merge doesn't work properly.

@dcastil
Copy link
Owner

dcastil commented Aug 29, 2023

Hey @Senneer! 👋

When the group isn't in the theme, then you need to check the default config and find the class group ID. For fontSize it's here: https://github.com/dcastil/tailwind-merge/blob/v1.14.0/src/lib/default-config.ts#L619

So your config should look like this:

const twMerge = extendTailwindMerge({
    classGroups: {
        'font-size': ['text-custom-1', ]
    }
})

@github-actions
Copy link

This was addressed in release v2.0.0.

@dcastil dcastil added the context-v2 Related to tailwind-merge v2 label Oct 30, 2023
@noorvir
Copy link

noorvir commented Dec 30, 2023

I had to add an extra extend in the call to extendTailwindMerge to get it to work:

export const twm = extendTailwindMerge({
  extend: {
    classGroups: {
      'font-size': [
        'text-h1',
        ...
      ],
    },
  },
});

@dcastil
Copy link
Owner

dcastil commented Dec 31, 2023

@mrleblanc101
Copy link

mrleblanc101 commented Mar 11, 2024

I was wondering why bg-stone-100 bg-[url(somefile)] was overwritten and would result in a white background (from the parent element).
Changing for bg-stone-100 bg-[url:url(somefile)]. I wonder if for "URL" the prefix is really necessary ?
Can't it be determined with heuristic ?

@dcastil
Copy link
Owner

dcastil commented Mar 14, 2024

Hey @mrleblanc101! 👋

Just tested it and I get the result twMerge('bg-stone-100 bg-[url(somefile)]') === 'bg-stone-100 bg-[url(somefile)]' in v2.2.1. tailwind-merge indeed uses url(…) to detect bg-images (code) and you shouldn't need to use the url: tag for them. Could you try to create a reproducible example and open a new issue with it?

@mrleblanc101
Copy link

@dcastil Might be a tailwind issue then.
Without the url:url() prefix, it would generate a background instead of a background-image, so the background-color longhand was overwritten by the background shorthand.
I'll look into it and provide a repro if needed or open a new issue on the tailwind repo.

@mrleblanc101
Copy link

mrleblanc101 commented Mar 17, 2024

@dcastil
Here is the repro: https://codesandbox.io/p/sandbox/infallible-dhawan-nvhf6t?file=%2Fsrc%2Findex.ts%3A23%2C57-23%2C82

It looks like tailwind-merge thinks that bg-[center_right_0.75rem] will overwrite my bg-stone-100.
But this seems incorrect, as tailwindcss properly infer that bg-[center_right_0.75rem] will results in background-position
I don't know if tailwindcss updated/improved they heuristic, but bg-[center_right_0.75rem] did not need the position: label like so bg-[position:center_right_0.75rem] to render as background-position longhand instead of simply backgorund shorthand.

So, it's partially improper use from my part, but seems also like a bug in tailwind-merge because the same thing works without tailwind-merge.

I can create a seperate issue if needed.

EDIT: Updated with the proper link to the repro

@dcastil
Copy link
Owner

dcastil commented Mar 17, 2024

@mrleblanc101 background-position and background-size classes always need a label for tailwind-merge to know what to do. It's an annoying limitation but it's necessary because the values for both of the properties look very similar and sometimes even identical. You can read more on that in the docs: https://github.com/dcastil/tailwind-merge/blob/v2.2.2/docs/limitations.md#you-need-to-use-label-in-arbitrary-background-position-and-background-size-classes

@mrleblanc101
Copy link

@dcastil But why and how does Tailwindcss infer it correctly ? Should tailwind-merge have the same inference ?

@dcastil
Copy link
Owner

dcastil commented Mar 17, 2024

Yes, I try to be close to Tailwind's implementation. It's not always possible to use the exact same approach because tailwind-merge has much bigger constraints when it comes to how fast it needs to process data and how much code it can ship since it's typically within the frontend bundle. But the biggest obstacle is usually how much time I have to research and build.

In case you really want to have this feature, feel free to research how Tailwind detects which of the two it is and post a new issue with the info. I could pick it up from there or you could continue and submit a PR.

A good starting point is Tailwind's definition of the backgroundPosition plugin: https://github.com/tailwindlabs/tailwindcss/blob/d86fd0bb5b69c9aa5c75d4e78f8fe78969d6ff50/src/corePlugins.js#L1965-L1967

@mrleblanc101
Copy link

@dcastil Thanks for you explaination.
I did not know tailwind-merge was not an official Tailwindcss plugin since it's heavily featured in most YouTube tutorials, so thank you for your plugin.
I am not an avid Tailwindcss user, so I'll look this up later if I have time or let someone more interested look into it as this is a pretty minor annoyance, I just found it off after understanding the problem completely.

@mateuswolkmer
Copy link

mateuswolkmer commented May 27, 2024

Spent some time figuring this out, might help someone. Pay attention to the custom class you need to extend in the config, you need to type the whole class, while on tailwind.config you can forgo the prefix.

const tailwindConfig = {
  theme: {
    extend: {
      fontSize: {
        'xs': ...,
        'sm': ...
      }
      ...
const twMergeConfig = extendTailwindMerge({
  extend: {
    classGroups: {
      'font-size': [
        'text-xs', // <-- include 'text'
        'text-sm',
        ...

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

Successfully merging a pull request may close this issue.

8 participants