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

Adds class directive shortcut and encapsulate styles #1695

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

jacwright
Copy link
Contributor

@jacwright jacwright commented Aug 25, 2018

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 });.

<div class:active></div>

Also, classes added this way are encapsulated in the stylesheet.

Old

<div class="{active ? 'active' : ''}"></div>
<style>
.active {
  /* Will be removed from the stylesheet since the compiler can't know it is used */
}
</style>

New

<div class:active="active"></div>
<style>
.active {
  /* Will be kept and encapsulate styles */
}
</style>

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>
```
@jacwright jacwright changed the title Adds class directive shortcut Adds class directive shortcut and encapsulate styles Aug 25, 2018
@jacwright
Copy link
Contributor Author

Addresses #1694

@jacwright
Copy link
Contributor Author

Docs are written in sveltejs/v2.svelte.dev#351

@@ -1159,3 +1169,9 @@ const events = [
name === 'volume'
}
];

function camelCase(str) {
Copy link
Member

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.

Copy link
Contributor Author

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.

@PaulMaly
Copy link
Contributor

PaulMaly commented Aug 26, 2018

@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.

@Rich-Harris
Copy link
Member

Thanks!

Also, classes added this way are encapsulated in the stylesheet.

Err... you sure? 😎 It understands ternary expressions, and now that we're changing undefined attributes to be non-existent, we can add binary expressions as well. Here's a demo.

It could get smarter about classes that are concatenated — it doesn't know, for example, that an element with class="thing {active ? 'active' : ''}" could match .thing or .active or .thing.active, so it will just bail and keep all classes. I'll add an issue for that.

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.

@jacwright
Copy link
Contributor Author

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.

@@ -1159,3 +1168,7 @@ const events = [
name === 'volume'
}
];

function propertize(prop) {
Copy link
Member

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.

@Rich-Harris Rich-Harris merged commit c7d372c into master Aug 28, 2018
@Rich-Harris Rich-Harris deleted the class-shortcut branch August 28, 2018 19:10
@Rich-Harris
Copy link
Member

awesome 🍻

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

4 participants