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

Allow CSS custom properties by using style.setProperty() #127

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harrysarson
Copy link

@harrysarson harrysarson commented May 17, 2018

When using custom css properties the syntax style.property = 'value'does not work for custom css properties. Instead, using style.setProperty() allows custom css properties to be set.

I would like to argue that this is a bug fix rather than an enhancement as currently elm silently fails to add these CSS properties. I believe that, if this PR is not the way to go and elm does not want to support custom css properties, then it should throw an error rather than silently failing.

Documentation for style.setProperty():
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty

Fixes elm/html/issues/129

When using custom css properties the syntax `style.property = 'value'`
does not work for custom css properties. Using `style.setProperty()`
instead allows custom css properties to be used.

Fixes elm-lang/html/issues/129
@harrysarson harrysarson changed the title use style.setProperty() to set style properties. Allow CSS custom properties by using style.setProperty() May 17, 2018
Copy link

@Pilatch Pilatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@berenddeboer
Copy link

Great, would love to have this.

@tj
Copy link

tj commented Jul 2, 2019

👍 this would be nice, elm-css and others are cool but I think writing regular CSS is easier, much more flexible, and more portable. Personally I like to keep styles outside of my components aside from "structural" styles as well.

For reference React allows as well: https://github.com/facebook/react/pull/9302/files#diff-61273f1ad4383d5a3a802ac95031ade4R221

@evancz
Copy link
Member

evancz commented Apr 15, 2020

The question in elm/html#129 is about the fact that this seems to duplicate the existing ability to use variables. The first example given in the spec is:

:root {
  --main-color: #06c;
  --accent-color: #006;
}
/* The rest of the CSS file */
#foo h1 {
  color: var(--main-color);
}

You would probably then say h1 [ id "foo" ] [ text "header" ] in your code. Would the following code be equivalent though?

import Html exposing (..)
import Html.Attributes exposing (..)

mainColor = "#06c"
accentColor = "#006"

foo = h1 [ id "foo", style "color" mainColor ] [ text "header" ]

I think one of the weaknesses of HTML/CSS/JS is that they each have separate systems for modules and variables, and it would be a shame to duplicate that duplication in Elm.

What is the use case you have in mind that requires a second mechanism for variables?

@berenddeboer
Copy link

The use case for me is interfacing with existing CSS frameworks. From a pure Elm perspective I agree it duplicates functionality. But I think we will be using CSS frameworks for a long time. For example I use elm-mdc: we use the CSS from Google's framework, but have rewritten the JavaScript in Elm. The CSS in Google's Material Design for the Web is huge, and the amount of engineering and testing effort that goes into that is much more than the JavaScript side. Duplicating that in Elm does not seem to serve a purpose.

From a user perspective, silently failing, does not seem to be "by design", rather a side-effect of picking one way of setting a property instead of another.

I haven't looked at performance implications: if setProperty() is measurably slower, we can always call JavaScript from Elm for the cases where we need this. If setProperty() is faster, we should use it.

@charukiewicz
Copy link

Would the following code be equivalent though?

Setting an inline style is not equivalent to using CSS variables. Yes, the styles applied to the elements may be the same, but there are other trade offs beyond what the element looks like. Setting inline styles in Elm may give you the benefit of the compiler, but makes it impossible to use a secure Content-Security-Policy, since inline styles requires including style-src: unsafe-inline in your CSP.

Here's more info about Content Security Policy from Mozilla. They consider a CSP "mandatory for new website" and "recommended for existing websites". Let me quote their recommendation as well:

It is recommended to start with a reasonably locked down policy such as default-src 'none'; img-src 'self'; script-src 'self'; style-src 'self' and then add in sources as revealed during testing.

The site observatory.mozilla.org is used for grading your website's overall security configuration. It's impossible to even get a perfect score if you include any unsafe CSP rules.

@harrysarson
Copy link
Author

As I said in the description:

I would like to argue that this is a bug fix rather than an enhancement as currently elm silently fails to add these CSS properties. I believe that, if this PR is not the way to go and elm does not want to support custom css properties, then it should throw an error rather than silently failing.

(Emphasis added).


I would completely understand if using css-variables in elm is considered "bad" and doing so causes an error. However, that elm silently fails in this case is definately a bug.

@Lattyware
Copy link

There is a discussion where I have listed my arguments in favour of supporting custom properties on the Elm discourse, and there I gave this example of interoperability with web components that is impossible without custom properties.

@annaghi
Copy link

annaghi commented Apr 23, 2020

Passing variables to CSS

This is another example of passing variables to CSS which I can share with limited access for now.

If I want to scroll horizontally in a grid table with sticky first column, I have to specify the width explicitly. This pen demonstrates the issue and the screencast below: https://codepen.io/annaghi/pen/oNgyLYw

codepen-Peek 2020-04-23 17-52

The following screencast shows the solution written in Elm, and you can see that I need to set the width based on columns count after reorganizing the table to guarantee the sticky first column while scrolling:

elm-Peek 2020-04-23 17-57

@lydell
Copy link

lydell commented Mar 20, 2021

Working on a custom virtual dom implementation, I tried .setProperty() out since I think it makes sense. Turns out that change is accidentally backwards incompatible! I noticed that while testing my implementation at work.

A text input was missing its rounded corners with my implementation. I’ve always written it like this (coming from a CSS background):

Html.Attributes.style "border-radius" "5px"

But it’s written like this at work (might be natural for someone who have done a lot of vanilla JS):

Html.Attributes.style "borderRadius" "5px"

Turns out doing element.style.borderRadius = "5px" and element.style["border-radius"] = "5px" is equivalent.

But element.style.setProperty("borderRadius", "5px") does not work! It mandates the “CSS spelling” with the dash: element.style.setProperty("border-radius", "5px").

How unfortunate.

It might be fixable, though:

if (property in element.style) {
	element.style[property] = value;
} else {
	element.style.setProperty(property, value);
}

@harrysarson
Copy link
Author

Ah man! That is so annoying! Javascript isba pita some times.

Thanks for spotting. I worry that the alternative might be slower (I guess up to two times?) as we have to interact with the DOM object twice. Maybe the overhead is lost in the noise though?

@harrysarson harrysarson marked this pull request as draft March 21, 2021 08:50
@Herteby
Copy link

Herteby commented May 3, 2022

Another workaround btw is to set the css variables like this:

attribute "style" "--color:blue"

@ChristophP
Copy link

@lydell @harrysarson
is it necessary to check whether the property exists in the DOM or could be just check the property name instead?

if (property[0] === '-' && property[1] === '-') { // check if prop starts with --
        element.style.setProperty(property, value);	
} else {
	element.style[property] = value;
}

If so, that could help avoid touching the DOM twice.

@lydell
Copy link

lydell commented May 16, 2022

@ChristophP You solution does not help with border-radius vs borderRadius, does it?

@ChristophP
Copy link

@lydell oh yeah, it does not. I was just looking at the multi DOM access parts. Guess I missed that it does the camel case / Kebap base checking as well.

@lydell
Copy link

lydell commented May 16, 2022

Wait, your solution would work I think! (I was wrong.) Because it does the same thing as now for everything except properties starting with --. Sounds promising!

@ChristophP
Copy link

@lydell Uhhm, yeah I guess 😅 You're right, if the prop doesn't start with -- everything would work the same.

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.

Support for CSS custom properties