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

Make classList and classes overloads of className #14

Closed
cmeeren opened this issue Aug 11, 2019 · 6 comments
Closed

Make classList and classes overloads of className #14

cmeeren opened this issue Aug 11, 2019 · 6 comments

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Aug 11, 2019

AFAIK className is a space-separated string of class names to apply to the component/element. Why not make classList (and classWhen) and classes overloads of className? That would improve discoverability, remove confusing extra names for what is essentially the same property, and be more in line with the goal of this project as I understand it.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 11, 2019

Also, it would remove a potential conflict/confusion with the classes prop that is used for more or less every Material-UI component.

@Zaid-Ajaj
Copy link
Owner

I liked the fact that classes is plural so that it takes a list of things, whereas className is supposed to take a single class(name). As for classWhen: the name implies the use of conditionals and will probably replace classList in the stable release.

This is my rationale for the choices, I will keep this (and the other issues) open for suggestions until the stable release

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 12, 2019

Yep, I just figured that since you do overloads anyway for improved usability, then using different names for what is essentially the same thing (a list of classes) was confusing to end users.

I guess my strongest point is this: It's not self-documenting that there are multiple ways to add classes; you have to read the docs to get it. Whereas if everything that adds classes are just overloads of className (which AFAIK is the canonical React name), then all users immediately see the various overloads (ways to add classes) when looking at the Intellisense.

Happiness +1 :)

@Zaid-Ajaj
Copy link
Owner

For readability: use classes, className and classWhen
For consistency & usability: use overloaded classes
For conformity (with React): use overloaded className

I am sure there is a way to strike a perfect balance but it is not an obvious choice to make here

Also I really really appreciate your feedback, helps a lot to moving forward with this library 😄

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 12, 2019

Yep, there are always tradeoffs... AFAIK the point of Feliz is discoverability/usability (it's certainly what brought me here), so I would definitely go with overloads in this case. I would be comfortable with overloaded classes, though IMHO it's better to follow established and google-able names, so I would go with className, which immediately indicates to users that it corresponds to the same React prop. Consistency, usability, and conformity. Yay!

Personally I don't find classes/className/classWhen more readable. classWhen in particular sounds very weird in my ears.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 12, 2019

Also I really really appreciate your feedback, helps a lot to moving forward with this library 😄

Happy to share my opinions! Note that they are in general strong opinions, weakly held. I can always change my mind in light of new realizations, information and compelling arguments. :)

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

No branches or pull requests

2 participants