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

Added tailwind-merge support #5209

Closed
wants to merge 1 commit into from
Closed

Conversation

bweis
Copy link
Contributor

@bweis bweis commented Nov 1, 2023

Previously, when attempting to override a tailwind class, the class merging algorithm would not take into account the priority of the tailwind base classes. A utility called tailwind-merge does this for you, along with maintaining the merging of standard classes that might be defined.

This has been brought up in a few other discussions. (See: #5144 and similar) Since it seems that there has been a big push to support tailwind, we decided to just hack away at this.

Obviously it would be very nice to not have to drill context into every component like that, however I decided to go that way since the president was set and I didn't want to influence design decisions as a first time contributer. In the event that you would like to avoid prop drilling, we could create a wrapped hook such as useMergeProps(props...) that returns a wrapped around the existing mergeProps() function. Ultimately, every file that was touched would still need to be touched.

Note

It works 100% if you rebase it on the tip of 10.0.7. Currently some styling issues exist on the top of master that seem to break components such as Editor.js.

Important changes

  • components/lib/utils/MergeProps.js now checks for the useTailwind context flag to determine what merging strategy to use.
  • The usePassthrough hook has also been updated to use the new merge functionality when the Provider has been enabled to do so. This works independently of the per component overrides.
const CustomTailwind = usePassThrough(
  {
    dropdown: {
      root: { className: 'w-36 md:w-36 bg-red-500 md:w-64' },
    },
  },
  {
    mergeSections: true,
    mergeProps: true,
    useTailwind: true,
  },
);
<PrimeReactProvider value={{ unstyled: true, pt: CustomTailwind, useTailwind: true }}>
  <AppRouter />
</PrimeReactProvider>
  • Documentation has been updated as have the types associated with the change.

Automated CodeMod

Just as an FYI, incase you want to change the shape of the API, I automated the code change with the following regex:
FInd \bmergeProps\(((?:[^;]+)|(?:\n))\); -> Replace mergeProps([$1], { useTailwind: context.useTailwind });

Copy link

vercel bot commented Nov 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
primereact ⬜️ Ignored (Inspect) Visit Preview Nov 1, 2023 7:04am

@bweis bweis force-pushed the tailwind-merge branch 2 times, most recently from e98d473 to 6385beb Compare November 1, 2023 07:03
@bweis bweis marked this pull request as ready for review November 1, 2023 07:05
@melloware melloware added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Nov 1, 2023
@melloware
Copy link
Member

Assigning to PrimeTek to review as this is a huge change.

@mertsincan
Copy link
Member

Hi,

Thanks a lot for your effort 👏 But, usePassThrough is a method used to perform basic merging and override operations. Frankly, we recommend that users create their own presets rather than using this method. Therefore, you can copy the Tailwind preset and create your base preset structure. We will update the documentation on this issue.

Unfortunately, close :( Thanks again!
Best Regards,

@mertsincan mertsincan closed this Nov 1, 2023
@bweis
Copy link
Contributor Author

bweis commented Nov 1, 2023

Hello! Thanks for the speedy response. Unfortunately I think that there might be a slight misunderstanding as to the purpose of the CL. Given how it currently works, the merge function ends up incorrect and the resultant CSS is wrong… The state of the library now is that Tailwind support is broken.

In short: the current merge function breaks a primary design philosophy for Tailwind and the library simply does not support Tailwind.

This does a good job explaining it, however I will paraphrase below.

Suppose that you have two tailwind classes:

.bg-red {
  background-color: red;
}
.bg-blue {
  background-color: blue;
}

And you have two components with the following stylings:

  <ComponentA className=”bg-red bg-blue/>
  <ComponentB className=”bg-blue bg-red”/>

What colors would these two components appear as?

Since CSS is Cascading StyleSheets, the blue will take precedence every single time. In face, once the .bg-blueclass is applied, there exists no way to apply the .bg-red class without polluting the namespace with !important everywhere in your passthroughs.

This is why tailwind-merge is basically required when you support manipulation of tailwind classes. The design philosophy of tailwind is that you keep the styling for your components close to the markup of the components. It is very common that you have a component and want to nudge padding a tiny but in a single instance, but not in all instances. The use of passthroughs in primereact is ideal for this however, if you were to try the following:

import Tailwind from 'primereact/passthrough/tailwind';
import { Paginator } from 'primereact/paginator';

const CustomTailwind = usePassThrough(
  Tailwind,
    {
    dropdown: {
      root: { className: 'bg-blue-500' },
    },
  },
  {
    mergeSections: true,
    mergeProps: false,
    useTailwind: true,
  },
);
return (
  <PrimeReactProvider value={{ unstyled: true, pt: CustomTailwind }}>
    <Paginator 
      pt={{
        RPPDropdown: { root: { className: 'bg-red-500' } },
      }}
    />
  </PrimeReactProvider>
);

If the CSS that gets created in the bundle is:

.bg-red-500 {
  background-color: red;
}
.bg-blue-500 {
  background-color: blue;
}

Then the dropdown’s background color will be blue, despite the override being set to red. In fact, with the current merge method, the resultant div on the page would look something like:
<div classNames=”bg-blue-500 bg-red-500”>Dropdown</div>

I hope that these examples and documentation will open you up to further discussion. This is definitely a deal breaker for myself and many other tailwind enthusiasts from using primereact. I am happy to hop onto a video call to discuss further if you need additional clarifying details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Pending Review Issue or pull request is being reviewed by Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants