-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Adds class directive shortcut and encapsulate styles #1695
Conversation
When no expression is used in a class directive the class name will be used to evaluate whether the class should be added/removed. E.g. the following will add the class "active" when you call `component.set({ active });`. ```html <div class:active></div> ```
Addresses #1694 |
Docs are written in sveltejs/v2.svelte.dev#351 |
src/compile/nodes/Element.ts
Outdated
@@ -1159,3 +1169,9 @@ const events = [ | |||
name === 'volume' | |||
} | |||
]; | |||
|
|||
function camelCase(str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how I feel about camel casing the class name to get the state field name. On the one hand, I can see how this would let you follow CSS class naming conventions more easily - but on the other, we're not doing this transformation anywhere else (on component prop names, etc.). I think I'm leaning towards saying that the class name and property name have to exactly match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use it when setting dataset
properties so I used it without considering. With dataset
however, that is all "under the hood". I didn't think about it being a new convention, and I understand your point.
Considering it further, I believe it is a useful convention and would lean towards keeping it, but I'm not passionate about it. I can remove it from here and sveltejs/v2.svelte.dev#351 if it is decided to remove it.
@jacwright Btw, every time I used old form, Svelte never remove that classes because of thinking them unused. Instead, it just switched off unused classes removal for that tag (or maybe even a component) to prevent any misunderstandings with dynamic data. I was very painful to me, because few times it happened in my Tailwind project, so whole TW styles included in the component. That’s why I don’t use any dynamic in class attribute anymore. |
Thanks!
Err... you sure? 😎 It understands ternary expressions, and now that we're changing It could get smarter about classes that are concatenated — it doesn't know, for example, that an element with I'm inclined to agree with @Conduitry about the camel-casing — I understand the motivation for it but I think it's easier to grok if everything is more literal. |
I removed the camelcase (adding code to ensure dashed shorthand still works) and updated the docs PR to reflect this. I also recommended in the docs to avoid shorthand for dashed classes, but it is supported because you know people will try. |
src/compile/nodes/Element.ts
Outdated
@@ -1159,3 +1168,7 @@ const events = [ | |||
name === 'volume' | |||
} | |||
]; | |||
|
|||
function propertize(prop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a utility function that does this, quotePropIfNecessary
.
awesome 🍻 |
When no expression is used in a class directive the class name will be used to evaluate whether the class should be added/removed.
E.g. the following will add the class "active" when you call
component.set({ active });
.Also, classes added this way are encapsulated in the stylesheet.
Old
New