-
Notifications
You must be signed in to change notification settings - Fork 80
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
base: master
Are you sure you want to change the base?
Conversation
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
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.
Looks good to me! Related documancy - https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty
Great, would love to have this. |
👍 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 |
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 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? |
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 |
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 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:
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 |
As I said in the description:
(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. |
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 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: |
Working on a custom virtual dom implementation, I tried 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 But How unfortunate. It might be fixable, though: if (property in element.style) {
element.style[property] = value;
} else {
element.style.setProperty(property, value);
} |
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? |
Another workaround btw is to set the css variables like this: attribute "style" "--color:blue" |
@lydell @harrysarson 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. |
@ChristophP You solution does not help with |
@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. |
Wait, your solution would work I think! (I was wrong.) Because it does the same thing as now for everything except properties starting with |
@lydell Uhhm, yeah I guess 😅 You're right, if the prop doesn't start with |
When using custom css properties the syntax
style.property = 'value'
does not work for custom css properties. Instead, usingstyle.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