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

Add primary and secondary colors as available theming colors to allow anyone to use their own custom color #40

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

dimitribarbot
Copy link

Same PR than before but without tailwind.config.js file.

For me, it's not really a good idea to remove theming example in the tailwind.config.js file as theming in Playground will not work by default when choosing primary or secondary but it's your component so it's your choice.

Please, tell me if it needs other changes.

Removing other `classNames` starting with `primary` and `secondary`
@JefteCaro JefteCaro merged commit 57c24d0 into onesine:master Jan 20, 2023
@dimitribarbot
Copy link
Author

Hello @JefteCaro, thank you for having merged my PR. However, why did you remove my changes regarding primary and secondary colors ? The feature has been completely removed, only the clean code part remained.

I see in your documentation that you're thanking Vue Tailwind Datepicker and when I look at what they've done regarding theming, I see that they're using a primary and secondary color configured as aliases in the tailwind.config.js file. Contrary to me, they're using the primary color as base color for the light theme and secondary one as base color for the dark theme, which is IMHO even better than what I proposed, but the idea remains the same.

I definitely need this feature to be able to use your component as it is a mandatory request for my project. How can we proceed so that I can avoid forking your repository and update it on a regularly basis ?

@JefteCaro
Copy link
Collaborator

Hi @dimitribarbot

It was a great idea, but, the primary and secondary classNames will be unused as we have default strings for the colors, which refers to the default colors by tailwind

Can you provide more details on how would you want to implement your primary and secondary colors from the Datepicker component as props?

@dimitribarbot
Copy link
Author

Hi @JefteCaro,

Thank you for your answer but I don't understand what you're saying.

If you're talking about the constants in constants/index.ts file, then I don't see any impact on the existing behaviour except for the overhead size it adds to your component output, which should be around 1 kB (unzipped). With a current index.esm.js of 73 kB, it does not seem to me that it is too much but we can divide it by 2 if we only add primary and remove secondary, or replace them with custom or rtd-primary for instance (if we want to do the same as in Vue Tailwind Datepicker where rtd stands for React Tailwind Datepicker).

If you're talking about the css built on your end users websites then as I mentionned before, if no primary or secondary colors are defined in the tailwind.config.js file then no classes are generated for them. There will not be any "unused classNames" as you said. On the contrary, if users are adding them in their config file, then they will be generated at build time but they will really be used by the datepicker as expected by this feature.

If you're talking about the fact that only default colors should be mapped to the datepicker primaryColor prop then I think it would be a bad idea to create other props for primary and secondary colors for at least 2 reasons:

  • I think it will add more overhead size to the component output than 1 kB because we have to add code to handle the choice between what is defined in the primaryColor prop and the new props everywhere colors are used in the code,
  • It may cause confusion on how to use primaryColor prop along with the new props because primaryColor is currently used to control the component colors, as the new props will do. What can be the expected behaviour if both primaryColor prop and the new props are filled at the same time ?

I think we can just consider the new colors as available colors for the primaryColor prop and update the documentation by saying this prop is accepting either a default Tailwind color, as you mentionned, or a custom one, but in the latter case developers must add config in their tailwind.config.js otherwise it will not work as expected.

You can remark that in Vue Tailwind Datepicker, this step is even mandatory, as explained in their doc. So here it would be better as by default no configuration is needed, but at the same time everyone can use their own custom color by adding it to the config file while setting the primaryColor prop to primary or secondary.

Otherwise, can you please share with me your point of view and explain to me why my solution is not ideal ?

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.

None yet

2 participants