-
-
Notifications
You must be signed in to change notification settings - Fork 84
Add CSS variables custom metrics #181
Add CSS variables custom metrics #181
Conversation
@LeaVerou WPT auto-updates to the latest stable automatically and is running the current version. |
@pmeenan Yup, I finally managed to run custom metrics on WPT and verified it is indeed running Chrome 84 :) |
@LeaVerou are there any good test cases you could share from the WPT results? Here's one from Smashing Magazine, but I'm not sure if this exercises the code sufficiently: https://www.webpagetest.org/custom_metrics.php?test=200729_EE_9aad73ca91dfdb7136100742b2412302&run=1&cached=0 For example, in the {
"element": {},
"children": [],
"declarations": {
"--bio-image-border-color": {
"value": "#fff"
},
"--bio-image-border-radius": {
"value": "11px"
},
"--bio-image-border-width": {
"value": "5px"
}
}
} Is there anyone you can recommend to review this from the CSS custom properties perspective? I'll give it a thorough review from the WPT/HA integration perspective. So far the results look reasonable. One suggestion I have is to combine this with the |
No, that is certainly not normal, I will investigate.
@tabatkins would be the perfect person for this, but not sure if he has the time to be involved.
I thought we could keep that for smaller metrics, and keep the Sass and CSS vars ones in their own files, since they are fairly large-ish (200-300 loc). Would that be ok? |
SGTM. |
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.
LGTM modulo empty element
feedback.
FWIW this seems to be a WPT issue, the code runs fine from a console
It's not just the empty element, the empty children is far more concerning. |
Alright, turns out that was actually an easy fix, I just wasn't using my own serialization function, so the replacer argument that removed empty arrays and serialized elements into nice |
Tested your site on WPT: https://www.webpagetest.org/custom_metrics.php?test=200730_V9_a35832e21be04e79bee4559aa1bc0794&run=1&cached=0. The newly fixed For your queries that depend on the |
It’s not newly fixed, it’s just that the previous output included it when empty (and it is frequently empty). I did mention that the output can get large with overly liberal selectors using custom properties ( |
|
||
if (ancestor) { | ||
let o = map.get(ancestor); | ||
o.children.push(obj) |
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.
Do you have any ideas on how I could compress/eliminate them?
First thing that comes to mind is serialize the obj
, save it in a Set, and test to see if each new object already exists before appending to o.children
.
Or check the children
array itself:
if (!o.children.find(child => JSON.stringify(child) == JSON.stringify(obj)))
The object properties may not be serialized in the same order, so this may not be a very robust option.
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.
@LeaVerou I'll merge this as-is to make sure that we get it into the sync tomorrow. If you're able to optimize this before the end of the month, feel free to file a PR and we'll try to fit it in.
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.
Thanks for merging!
First thing that comes to mind is serialize the obj, save it in a Set, and test to see if each new object already exists before appending to o.children.
The structure is built recursively, so at the time of that push, obj
is not necessarily in its final state. But I could do something along those lines after the tree is built.
While I can't commit to anything large, I can do small reviews. I can't tell from this thread what I'm being asked to review, tho. A little help? |
Thanks Tab! |
Details here: LeaVerou/css-almanac#1 (comment)
Please note that this is written assuming a recent version of Chrome. If WPT runs an older one, please let me know so I can make changes.
Progress on HTTPArchive/almanac.httparchive.org#898