Change classMap to set classes correctly for SVGs #1070
Conversation
Co-Authored-By: Adam Vigneaux <adam@adamvig.com>
Co-Authored-By: Adam Vigneaux <adam@adamvig.com>
jridgewell
force-pushed
the
fix-class-map-svg-compat-2
branch
from
db0deb3
to
e68f3cd
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Hi @AdamVig, can you comment "@googlebot I consent" so we can reuse your commit? |
|
@googlebot I consent |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@jridgewell Thanks for revisiting this, it has been on my list since my original pull request. |
There was a problem hiding this comment.
Yeah, thanks a lot for taking this up @jridgewell!
I'm a little bummed about just dropping down to setting the class attribute now, but we never measure the supposed perf benefit of manipulating the classList directly, so it was an unverified claim. There should be some benefit left from the deep dirty-checking on the classInfo object.
Given this is just using setAttribute() I think there's a simplification here, so I'd like to see that tried first.
src/directives/class-map.ts
Outdated
| @@ -23,20 +22,21 @@ export interface ClassInfo { | |||
| * Stores the ClassInfo object applied to a given AttributePart. | |||
| * Used to unset existing values when a new ClassInfo object is applied. | |||
| */ | |||
| const previousClassesCache = new WeakMap<Part, Set<string>>(); | |||
| const previousClassesCache = new WeakMap<Part, Map<string, unknown>>(); | |||
There was a problem hiding this comment.
I think this deserves a comment explaining what's happening, since the key in the Map is now either a class name or a static string...
jridgewell
force-pushed
the
fix-class-map-svg-compat-2
branch
from
084c9a7
to
486f633
Compare
| const cssInfo = {foo: 0, bar: true, zonk: true}; | ||
| render(svg`<circle class="${classMap(cssInfo)}"></circle>`, container); | ||
| const el = container.firstElementChild!; | ||
| assert.isFalse(el.classList.contains('foo')); |
There was a problem hiding this comment.
classList needs to be replaced with className.split(' ')... for SVG elements on IE
| assert.isFalse(el.classList.contains('aa')); | ||
| assert.isTrue(el.classList.contains('bb')); | ||
| renderClassMapStatic({}); | ||
| assert.isFalse(el.classList.contains('aa')); |
There was a problem hiding this comment.
This seems bad to me, but I guess it's current behavior. I'd much rather have the behavior that the output is the same for the same input, regardless of order of calls (with the exception of out-of-band classList mutations).
There was a problem hiding this comment.
Agreed. Wanna cut a v2 anytime soon?
There was a problem hiding this comment.
Not particularly :)
I'd want to move further on SSR and add a whole lot of benchmarks before that at least.
This fixes the
classMapdirective inside SVGs by avoidingclassName, and avoidingclassListwhich isn't supported in IE11 SVGs.This is another attempt at #932. I've reused @AdamVig's commit to give him the commit stats.
Fixes #930.