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

[Fix] inputClassName, containerClassName, and toggleClassName accept functions... #80

Merged
merged 6 commits into from
Apr 3, 2023

Conversation

giero
Copy link
Contributor

@giero giero commented Mar 2, 2023

… to override component className (#64)

This is another (i think better) approach after #68.

Now we will be able to do sth like this:

inputClassName={(className) => twMerge(className, "dark:bg-blue-100")}

and twMerge is no longer needed inside react-tailwindcss-datepicker :)

Also fixed some minor bugs and removed unnecessary code.

@onesine
Copy link
Owner

onesine commented Mar 3, 2023

@giero, I think we are starting to have too many props for classes. We can group them all in the classNames props. But we'll have to leave the ones that are there so that those who already use the library with these props won't have problems with updates.

Thank you for this PR

@giero
Copy link
Contributor Author

giero commented Mar 3, 2023

If you want I could adapt classNames for the same purpose as my changes for individual fields and restore them to strings only (to avoid problems with updates)?

@onesine
Copy link
Owner

onesine commented Mar 3, 2023

OK, that would be great.

Copy link
Owner

@onesine onesine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same problem can occur if the user uses strings on the inputClassName, containerClassName and toggleClassName props.

return typeof toggleClassName === "function"
? toggleClassName(defaultToggleClassName)
: typeof toggleClassName === "string"
? `${defaultToggleClassName} ${toggleClassName}`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing for this line

typeof containerClassName === "function"
? containerClassName(defaultContainerClassName)
: typeof containerClassName === "string"
? `${defaultContainerClassName} ${containerClassName}`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there will always be problems with this line.

If in containerClassName we have text-red-400 then it will conflict with text-gray-700 of defaultContainerClassname.

Copy link
Contributor Author

@giero giero Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that's why I offered to use tailwind-merge to solve this issue in #68. To avoid adding it to the library, containerClassName (and other "class names") became functions to allow users to use twMerge (or other library) outside the datepicker.

But yes - if containerClassName is a string there still will be issue with this ... I left this to avoid BC break.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case containerClassName is a

  • string we want to style the container with only its own utility classes then we don't need twMerge
  • function we want to add utility classes to the existing ones, then we may need twMerge to avoid conflicts.

When containerClassName is not defined, we use the default style.

return typeof inputClassName === "function"
? inputClassName(defaultInputClassName)
: typeof inputClassName === "string"
? `${defaultInputClassName} ${inputClassName}`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing for this line

@JshGrn
Copy link

JshGrn commented Mar 6, 2023

Whats the latest with this because I do not want to use your theming classes...

@onesine
Copy link
Owner

onesine commented Mar 6, 2023

In case containerClassName is a

  • string we want to style the container with only its own utility classes then we don't need twMerge
  • function we want to add utility classes to the existing ones, then we may need twMerge to avoid conflicts.

When containerClassName is not defined, we use the default style.

@giero

@giero
Copy link
Contributor Author

giero commented Mar 27, 2023

@onesine I updated this PR to apply your suggested changes. So providing inputClassName/containerClassName/toggleClassName as strings will override existing default styles. Using them as functions will allow to use default styles with some external class-merge-tool (like twMerge) by user.

@onesine
Copy link
Owner

onesine commented Apr 2, 2023

I should see this but I couldn't. So sorry @giero . You can resolve the conflicts and republish.

@giero giero requested a review from onesine April 3, 2023 07:33
@onesine onesine merged commit b4c780f into onesine:master Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants