-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Report: collapse aggregations when all audits pass #1742
Conversation
sure it uses them now ;) this is awesome though! |
Totally on board with the collapsing. Nice. Regarding scoring, are you saying we'll now have scores for "Best practices" and "fancier stuff"? |
@@ -387,11 +412,11 @@ | |||
"weight": 1 | |||
}, | |||
"critical-request-chains": { | |||
"expectedValue": 0, |
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.
I thought this one actually was numeric, is it not?
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.
The rawValue
that we display is numeric, yes. But we treat the audit as a pass/fail, which means expectedValue
needs to be a boolean. AFAICT, expectedValue
should really be expectedType
.
Sorry. There's still only one score in the report (PWA). The difference with this PR is that it calculates an overall score for each subitem. That's not surfaced in the report though. Before (all Now: |
After playing with it for a bit, I really like it, but the UX of the cards/boxes could use some work since I don't think it plays well with the collapsed report. Everything is rather succinct and cleanly laid out and the three large uncollapsible boxes really jump out. Can we have those collapse too? |
We can play with collapsing those too. Do you think if we use more cards in the report, it'll feel less jarring? Personally I like seeing them b/c it's something other than text. |
Yeah that's a fair point. Right now they just really stick out. If we used them more throughout I'd probably feel differently, but probably need to a strike a balance to prevent wall of text becoming wall of boxes :) |
Agree with that, but if those numbers are in good shape then it's not adding much value to always show them. |
2e5084c
to
c1ff285
Compare
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.
SavageJ25 approves this pull request.
I mean... I'm sure he does, too.
(Thx for handling all the back n forth like a pro)
@patrickhulce you happy too? |
Oh sorry yep! |
R: @patrickhulce @paulirish
In the act of de-cluttering, this PR addresses some of of #1613 and implements the proposed solution in #1613 (comment).
Now, if all audits pass, the entire aggregation is collapsed. If one or more audits fail, the aggregation is opened up.
Before:
After:
when an audit fail:
BTW, the aggregation icons are slightly bigger than the subaudits. I thought the size difference was good but I'm happy to make them all the homogeneous.
expectedValues
/weights
are important and we'll need to use them after all :) aggregate.js uses them to type check and calculate an overall score based on the weights.