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 custom classNameMergeFunction configuration to ptOptions #5215

Closed
wants to merge 7 commits into from

Conversation

bweis
Copy link
Contributor

@bweis bweis commented Nov 1, 2023

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

This enables the usage of libaries such as tailwind-merge to more
sensibly merge classNames together when useing Tailwind.
Copy link

vercel bot commented Nov 1, 2023

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 Dec 16, 2023 0:36am
primereact-v9 ⬜️ Ignored (Inspect) Visit Preview Dec 16, 2023 0:36am

@bweis bweis marked this pull request as ready for review November 1, 2023 17:49
@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

This looks clever to me.

@melloware melloware added the Component: Tailwind Tailwind specific issue label Nov 5, 2023
@bweis
Copy link
Contributor Author

bweis commented Nov 7, 2023

Is there any other context that I can add here to move this forward?

@melloware
Copy link
Member

@gucal it would be great if this was included in 10.0.10 or probably be 10.1.0 to be safe with semver as this is a feature a lot of users are asking for.

@gucal
Copy link
Contributor

gucal commented Nov 10, 2023

Thank you for your effort.
We will review and get back to you as soon as possible. We plan to complete it at the next or later milestone.

Thanks!

@MarcusResell
Copy link

This is exactly what would unblock us from moving to version 10 and also utilise unstyled mode with tailwind. Is there any update on the timeline for this? Unfortunate that the release for the css variables were pushed a long with this bottleneck for Tailwind and unstyled mode. We will unwillingly have to stick with scss themes in our remake

@melloware
Copy link
Member

@bweis any chance you could update the conflicts? If not I understand since its been a long time to merge this PR but I think I am ready to merge it for 10.4.0 it fixes so many TailWind issues

@melloware
Copy link
Member

Implemented with: #5755

@melloware melloware closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Tailwind Tailwind specific issue Status: Pending Review Issue or pull request is being reviewed by Core Team
Projects
None yet
5 participants