lighthouse-ci
lighthouse-ci copied to clipboard
Add CLS to metric graphs
Describe the solution you'd like CLS has a major weight in the score, is a Core Web Vital, and should be visually trackable in the dashboard. No design was ever created for it given that it was added to Lighthouse after LHCI was a thing and it's on another scale compared to all the other metrics.
Describe alternatives you've considered None.
I made a start on this but I wasn't able to get the project running locally yet 😞
https://github.com/GoogleChrome/lighthouse-ci/compare/main...dkarzon:cls-graph
This should be the API and UI components of it so new builds will have the statistics for it generated and the graphs are added to the dashboard but I wasn't sure how to do the database migration part to get the statistics for existing builds and I haven't been able to test it.
Thanks very much @dkarzon! I think the complexity here though is in how to design the dashboard to avoid the odd numbered graph issues while still visualizing on smaller desktop viewport sizes.
@patrickhulce what do you mean by odd numbered graph issues
here?
@patrickhulce what do you mean by odd numbered graph issues here?
Simply that we've got a nice two column layout going at the moment that fits nicely in a single viewport. If we just added a 3rd graph with CLS it would be thrown off a bit and require some scrolling. That's the simple solution here and given the lack of movement here maybe that's just the best course of action for now 🤷
@patrickhulce I am able to get upto this point where I can render the graph for CLS and as first graph has multiple metrices rendered, I tried to move it down to occupy more space.
However, there seems to be an existing bug which prevents the tooltip (hover-card) from being rendered for the graph which is rendered last (in this screenshot graph with FCP/LCP/TTI/SI). If I move the CLS graph to the last, it stops rendering hover-card for it. I am still trying to debug it.
@patrickhulce did you get chance to look at this layout?
if we flip the FCP/LCP graph above the other two then LGTM :)
if we flip the FCP/LCP graph above the other two then LGTM :)
Yes, that also looks good and infact I tried that first. This layout I tried to see if there is bug already existing or is it introduced because of my changes. The bug I am talking about is:
However, there seems to be an existing bug which prevents the tooltip (hover-card) from being rendered for the graph which is rendered last (in this screenshot graph with FCP/LCP/TTI/SI). If I move the CLS graph to the last, it stops rendering hover-card for it. I am still trying to debug it.
@patrickhulce do we have any guide for how to fix unit tests for these changes? I am seeing some unit tests failing after introduction of new graph.
Which unit tests? Link to log output would be great
@patrickhulce I am just trying to run npm run test
after my changes and it is breaking some of the existing tests.
I understand that, but I can't help unless I know which tests and how they are being broken :)
@patrickhulce here are few of the tests (snapshots
mostly) which are failing:
OK and have you opened the diff to see if what has changed is expected? If you're changing the UI, updating snapshots is normal :)
I didn't try to open diff yet as I didn't realize that I can see the diff image as well. On the second point, is there any guide how to update snapshots?