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

Drop camelCase to kebab-case auto-conversion for CSS classes #52

Closed
msaraiva opened this issue Mar 2, 2020 · 14 comments
Closed

Drop camelCase to kebab-case auto-conversion for CSS classes #52

msaraiva opened this issue Mar 2, 2020 · 14 comments

Comments

@msaraiva
Copy link
Member

msaraiva commented Mar 2, 2020

The reason I added this feature was to be able to work nicely with css libs like bulma which has naming conventions for classes mostly as is-* and has-* and since atom keys cannot contain - without wrapping it in "...", it felt like a good addition at the time. The rationale behind it was that if you don't want the auto-conversion, you could just use keys as strings instead of atoms:

<!-- Using atoms: `isDanger` is auto-converted to "is-danger" -->
<div class={{ isDanger: true }}>

<!-- Using strings: This is NOT auto-converted. It keeps whatever value the key has -->
<div class={{ "isDanger" => true }}> 

However, I'm afraid no matter how good this feature is documented (currently is not), people will still get confused, especially newcomers. So in order to avoid confusion, I think we could just drop this feature and always use the original value. WDTY?

BTW, as far as I can remember some JS frameworks have similar auto-conversion feature.

@wrren, @brainlid, @mazz-seven or anyone interested. I'd love to hear your thoughts about this one.

Original discussion can be found here.

@wrren
Copy link
Contributor

wrren commented Mar 2, 2020

Personally I quite like the auto-conversion feature, and I think it makes perfect sense when you think about conventions around CSS classes. As long as it's called out in the documentation clearly I think it's fine.

Alternatively you could use underscore-case -> kebab-case conversion instead; if anything I think that makes even more sense since you're going from idiomatic elixir naming to idiomatic CSS naming.

@wrren
Copy link
Contributor

wrren commented Mar 2, 2020

Another option would be to make this a config setting, or something passed as a __using__ option? e.g:

use Surface, convert_classes: false

@lnr0626
Copy link
Collaborator

lnr0626 commented Mar 2, 2020

for what it's worth, I prefer to just specify the css class name without any conversion

the underscore to kebab case conversion would cause issues with BEM style CSS if i'm not mistaking what you're suggesting

@neilberkman
Copy link

the underscore to kebab case conversion would cause issues with BEM style CSS if i'm not mistaking what you're suggesting

We're encountering this now: #50 (comment)

@lnr0626
Copy link
Collaborator

lnr0626 commented Mar 2, 2020

I recognize that those names wouldn't be idiomatic elixir; however I feel like the edge cases get fairly extensive when we start converting things; and as the names are technically css and not elixir the non-idiomaticness doesn't bother me too much

@brainlid
Copy link
Contributor

brainlid commented Mar 2, 2020

@msaraiva Speaking to your point about feature causing confusion, I've had to pull up the docs to check how to do it multiple times because it doesn't feel natural to me. So I'm sympathetic to that concern.

I do like the ability to include a class with a boolean value (or function call) so I want the ability to remain, but the implementation and API are fine to change for me. I'm not using any CSS libraries that have issues with it.

@msaraiva
Copy link
Member Author

msaraiva commented Mar 2, 2020

@neilberkman just to make it clear, you should be able to use "clipboard__icon" today if you write the key as a string, so:

<div class={{ "clipboard__icon" => true }}>

should be translated to:

<div class="clipboard__icon">

Pay attention that "clipboard__icon": true is different from "clipboard__icon" => true. In the former the key is an atom, in the latter, the key is a string.

In case the above doesn't work as expected, we have a bug so please let me know.

@msaraiva
Copy link
Member Author

msaraiva commented Mar 2, 2020

Another option would be to make this a config setting

@wrren I would avoid a config for that as much as possible since it doesn't solve the issue of confusion. I'm inclined to drop the feature unless we can make sure that improving the docs/examples will be enough.

I'll merge #51 in the meantime and wait for more feedback to make a final decision about dropping the feature.

Thank you all for your comments ❤️

@mazz-seven
Copy link
Contributor

Personally I didn't used the auto-convert. I think the argument to support isn't sufficient. There are many ways to write css class names, Im using tailwindcss for example. So to support one way of writing class names that cause so many problems seems unnecessary. Hope my input helps :)
On related topic, can I do something like that?

some_classes = "class_1 class_2 .." 
<div class={{ ^some_classes => true }}>

@msaraiva
Copy link
Member Author

msaraiva commented Mar 9, 2020

@mazz-seven thanks for the input.

Regarding the suggested code, it's currently not possible. The closest thing I can come up with, in case you'll alway use true, is:

some_classes = [:class_1, :class_2, ...] 
<div class={{ some_classes }}>

However, I'm not sure which other cases you want to cover, so this is probably not what you're looking for.

@gdub01
Copy link

gdub01 commented Mar 23, 2020

I'd vote for no automatic conversion.

Absinthe has auto camelCase conversion for graphql queries and there have been many questions from people wondering what happened to their field names. Absinthe allows users to override it, and have good docs, but it has still surprised many people.

I understand why they did it, and appreciate it in their case.. but I would not enjoy it as much for css... I'd just prefer it to be what I think it is.

@mathewdgardner
Copy link
Contributor

I'm in favor of not automatically kebabing or to at least provide an escape hatch. I am using an existing style sheet that I'm writing components for and it mixes use of kebab and camel cases.

@msaraiva
Copy link
Member Author

msaraiva commented Mar 28, 2020

All right, folks!

I've decided to drop the automatic camel-to-kebab conversion. For those that like this feature, I'm willing to find another way to provide similar behaviour by maybe adding a built-in function or some kind of modifier to make its use explicit.

The next alpha version should be released already without it.

Thank you all for the fantastic feedback!

@msaraiva
Copy link
Member Author

msaraiva commented Apr 3, 2020

Resolved in #62. Thanks @zamith!

@msaraiva msaraiva closed this as completed Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants