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

CSS "!important" not supported in Svelte "style" #7365

Closed
bluepuma77 opened this issue Mar 13, 2022 · 7 comments
Closed

CSS "!important" not supported in Svelte "style" #7365

bluepuma77 opened this issue Mar 13, 2022 · 7 comments
Labels
compiler Changes relating to the compiler feature request

Comments

@bluepuma77
Copy link

Describe the bug

It seems CSS !important is not support in the Svelte style attribute.

<h1 style:background-color={color}>works</h1>
<h1 style:background-color="{color}">works</h1>
<h1 style:background-color="{color} !important">fails</h1>

But it is of course important if you want to overwrite a style.

Reproduction

https://svelte.dev/repl/152608cb822e41e68ce43522ef96d875?version=3.46.4

Logs

none

System Info

Just REPL

Severity

blocking an upgrade

@dummdidumm dummdidumm added feature request compiler Changes relating to the compiler labels Mar 13, 2022
@bluepuma77
Copy link
Author

Ok, maybe this is not important. Plain HTML style works:

<h1 style="background-color:{color} !important">!important</h1>

@adiguba
Copy link
Contributor

adiguba commented Mar 13, 2022

Hi,

It seems to produce the following code :

let style_background_color = `${/*color*/ ctx[0]} !important`;
// ...
set_style(h1, "background-color", style_background_color, false);

=> "!important" should not be present in the property argument, but should match the boolean's value of the 4th parameter.

In this specific case, I think it should produce the following code instead :

set_style(h1, "background-color", /*color*/ ctx[0], true); // important = true

But this is not enought because the !important can be on the "color" variable, like this :

<script>
	export let color = 'red !important'
</script>

<h1 style:background-color="{color}">!important</h1>

And in this case, set_style() should detect the presence of "!important" in the value...

@andirady
Copy link

I prefer this to be not supported and documented. It should throw an error telling you should not put "!important" in the value.
Better syntax would be

<h1 style:background-color|important={color}>works</h1>

@baseballyama
Copy link
Member

I prefer this to be not supported and documented. It should throw an error telling you should not put "!important" in the value.
Better syntax would be

Could you please explain to me why the above syntax is better?

Personally I prefer below.

<!-- style:property -->
<h1 style:background-color="{color} !important">foo</h1>

<!-- natural style attribute -->
<h1 style="background-color: {color} !important">foo</h1>

This is much similar to the natural style attribute.
I think one of the good point of Svelte is HTML superset-like.
We can implement Svelte with HTML, JavaScript, CSS, and a little knowledge of Svelte.
So it is great that it can be written in the same way as natural HTML as much as possible.
So personally I prefer this.

@gtm-nayan
Copy link
Contributor

I can see the rationale behind it
!important is a modifier in a sense that it modifies how the style is applied
It'd be consistent with how modifiers work for event handlers

Also, taking a quick look through the PR it seems like it's a compile time check instead of a runtime check as mentioned in #7365 (comment)

Going that route, a directive modifier would be cleaner and also could be easier to statically analyze than "!important" as part of the value itself.

Although I don't know enough about the CSS spec so the pipe might be a valid character in custom property names, in which case the |important syntax would be completely off the table.

@baseballyama
Copy link
Member

Thank you for explaining!
I'm leaning toward this opinion now.

I think controlling !important by a condition at runtime is a niche case and there is workaround.
And amount of runtime work should be less for performance.
So |important syntax is enough for covering actual use-case and it doesn't increase amount of runtime work.
I think this is enough reason why we will introduce |important.

the PR it seems like it's a compile time check instead of a runtime check

Yeah, in this case, I think we need to modify around set_style function in runtime also.

Although I don't know enough about the CSS spec so the pipe might be a valid character in custom property names, in which case the |important syntax would be completely off the table.

I checked it and custom property name can not use |.

@Conduitry
Copy link
Member

This is supported now via an |important modified (style:foo|important={foo}) in 3.52.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Changes relating to the compiler feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants