fix: setting styles with attribute for svg elements #565
Conversation
NodeNS elements require a slightly different method to consistently update styles than Html elements. Specifically, NodeNS elements will require the atttribute property with the "class" string.
There was a problem hiding this comment.
I made a PR adding a couple of tests against this PR -- hope that's alright with you!
This approach looks reasonable to me as long as:
- we're sure that namespaced nodes will only ever be SVG nodes
- we are fine with a runtime exception occurring if a user every accidentally uses
Html.Styled.Attributes.classwith anSvg.Stylednode (UsingHtml.Styled.Attributes's css breaks Svg.Styled at runtime #503 doesn't want to get closed, I guess!) - we are fine with elm-test's html helpers not being able to test SVG classes (for this one, I think we either need to fix it in elm-test, or we have to be fine with it)
examples/html-styled/Main.elm
Outdated
| , btn [ onClick DoSomething ] [ text "Click me!" ] | ||
| , StyledSvg.svg [ SvgAttribs.css [], SvgAttribs.width "100", SvgAttribs.height "100" ] [ StyledSvg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ] | ||
| , StyledSvg.svg [ SvgAttribs.class "some-whatever-NS-class", SvgAttribs.css [ opacity (num 0.5) ], SvgAttribs.width "100", SvgAttribs.height "100" ] [ StyledSvg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ] |
There was a problem hiding this comment.
The circle is a bit low-contrast against the background.
How would you feel about using bright red instead?
| , StyledSvg.svg [ SvgAttribs.class "some-whatever-NS-class", SvgAttribs.css [ opacity (num 0.5) ], SvgAttribs.width "100", SvgAttribs.height "100" ] [ StyledSvg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ] | |
| , StyledSvg.svg | |
| [ class "some-whatever-NS-class" | |
| , SvgAttribs.css [ color (rgb 256 0 0) ] | |
| , SvgAttribs.width "100" | |
| , SvgAttribs.height "100" | |
| ] | |
| [ StyledSvg.circle | |
| [ SvgAttribs.cx "50" | |
| , SvgAttribs.cy "50" | |
| , SvgAttribs.r "40" | |
| , SvgAttribs.fill "currentcolor" | |
| ] | |
| [] | |
| ] |
There was a problem hiding this comment.
This is fantastic, many thanks for opening the PR I shall review and merge post haste.
On your points -
-
The changes I made definitely only ever deal with NodeNS and KeyedNodeNS types so I think we are good there.
We are limited in testing this in the package with elm-test but I don't see why we couldn't test with something like playwright. I did this for elm-select and it works great. Perhaps a little overkill but we could raise an issue to explore how to test the svg class behaviour. -
The current runtime exception regarding the svg styles already exists so whilst we haven't fixed the issue, we haven't made it worse.
-
I think it makes sense to patch elm-test to handle svg classes. Was it ever actually working correctly? Patching elm-test does address the testing situation in the first point however so win win.
There was a problem hiding this comment.
Yay, glad you like it!
-
I haven't ever used playwright -- looks great! Maybe Percy could be neat to have in place too?
-
True! 🙈
-
No, as far as I can tell, I don't think elm-test with SVG classes was ever working 🤔
There was a problem hiding this comment.
Cool! Changed to a high contrast colour as per your request, I guess we just wait for @rtfeldman to have a look.
I'll open an issue in elm-test to patch svg testing. Never looked at that project so should be fun!
There was a problem hiding this comment.
Looks like an issue in elm-test exists elm-explorations/test#134.
Had a super quick look at the project and I think a fix is relatively straightforwards. (I will regret saying that)
Adds regression tests for rtfeldman#564
There was a problem hiding this comment.
This looks good to me! Thanks for the review and the extra improvements @tesk9!
@Confidenceman02 are you ready for merge and release, or want to make any final changes?
|
Happy to merge and release @rtfeldman. |
fixes #564
Context
17.0.4 fixed a bug that was causing DOM exceptions by changing the way we were updating styles to use
VirtualDom.attribute "class" ...instead ofVirtualDom.property "className" ....This was due to svg elements not liking the
property "className"method which is a known phenomenon in the html spec.Some random stack overflow post that is probably true: https://stackoverflow.com/questions/43750395/how-to-add-a-class-in-a-svg-element-circle-using-javascript
However, whilst the pesky exception was fixed there were some unintended side effects you can read about in this issue #564.
Work completed
extractUnstyledAttributefunction to behave as it used to for html elements.extractUnstyledAttributeNSfunction to set styles for svg nodes using theattributemethod.The above changes focus on only fixing what was breaking, rather than changing the implementation for all elements.
Styled examples showing svg and html elements with their respective class properties.