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

Tailwind Merge #5755

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Tailwind Merge #5755

merged 1 commit into from
Jan 12, 2024

Conversation

melloware
Copy link
Member

@melloware melloware commented Jan 12, 2024

@bweis This is the next iteration on #5209

Fix #5144
Fix #5564
Fix #5570

New Context

Some CSS Class systems require a careful merging of classnames since there is no way to know what classnames have higher priority. This new configuration option will allow consumers of the library to assume the responsibility for merging these classnames. (ie: tailwind-merge for Tailwind)

After discussing in the discord, we determined this composable solution would be a better choice as to not constrain ourselves to tailwind support.

Problem that this is solving

This is an issue in broader CSS. But when consuming a component lib its not of large impact generally because you would have the component lib's CSS defined before your local, so your local CSS would take priority. However, when using tailwind this is not the case and is well documented by tailwind itself. Because of this many tailwind users rely on tailwind merge to resolve these conflicts.

Copying the preset and customizing it to your own liking is an partial option, however this does not solve the use case in which you either use the tailwind preset or your own and then later in your application want to customize a component using the pt={} props. This will never work with tailwind classes.

If a user uses the Tailwind preset or a custom PT config at the provider. But then wants to override a class of a specific component, this will not be possible for them to do with the existing APIs.

This change aims to fix that and is necessary for proper tailwind (or other css library) support.

As it stands before this PR, the paginator's bg color can never be reliably changed:

const customPT = {
  dropdown: {
      root: { className: 'bg-blue-500' },
    },
}
return (
  <PrimeReactProvider value={{ unstyled: true, pt: customPT}>
    <Paginator 
      pt={{
        RPPDropdown: { root: { className: 'bg-red-500' } },
      }}
    />
  </PrimeReactProvider>
);

With the proposed non-breaking changes in this PR you will be able to properly use talwind css in primereact. Without it tailwind support is not reliable for anyone that wants to modify any styling in components themselves beyond the provider passthrough.

Notable changes

  • The mergeProps function is now consumed via a hook useMergeProps so that context can be provided automatically.
  • The original mergeProps function has been made "package private" as to only be used by the ComponentBase.js.
  • Documentation has been added for the usage of this new configuration.

Remaining questions/notes

  • the usePassthrough "hook" is a misnomer and should really be renamed as mergePassthrough since it is not a hook in a traditional sense, more-so a utility function.

Previous PR Context

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.
    import { twMerge } from 'tailwind-merge';
    const CustomTailwind = usePassThrough(
    {
    dropdown: {
    root: { className: 'w-36 md:w-36 bg-red-500 md:w-64' },
    },
    },
    {
    mergeSections: true,
    mergeProps: true,
    classNameMergeFunction: twMerge,
    },
    );
```javascript
```javascript
import { PrimeReactProvider } from "primereact/api";

<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 });

@melloware melloware added the Component: Tailwind Tailwind specific issue label Jan 12, 2024
Copy link

vercel bot commented Jan 12, 2024

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

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
primereact ⬜️ Ignored (Inspect) Visit Preview Jan 12, 2024 2:37pm
primereact-v9 ⬜️ Ignored (Inspect) Visit Preview Jan 12, 2024 2:37pm

@melloware melloware merged commit 7b0f6ee into primefaces:master Jan 12, 2024
7 checks passed
@melloware melloware deleted the tailwind-merge branch January 12, 2024 14:39
@Aperrix
Copy link

Aperrix commented Feb 15, 2024

i'm trying to use classNameMergeFunction with Next 14 app router but sadely it doesn't support SSR and i can't use "use client" in my top level layout or i'll loose benefits of SSR in my entire app

image
image

@melloware
Copy link
Member Author

I would open a new ticket @Aperrix with Stackblitz reproducer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Tailwind Tailwind specific issue
Projects
None yet
2 participants