-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(redesign): score icons, audit group headers, colors #8222
Conversation
(just noticed I need to add back the description for clumps when expanded, ex: "Run these additional validators on your site to check additional SEO best practices.") |
looks good! haven't taken an deep look yet but the text color jumped out, is the split between icon and text colors for color contrast purposes in an upcoming PR? |
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.
last comments, otherwise LGTM
const expectedDescription = Util.UIStrings.lsPerformanceCategoryDescription | ||
// Replacing markdown because ".textContent" will be post-markdown. | ||
.replace('[Lighthouse](https://developers.google.com/web/tools/lighthouse/)', 'Lighthouse'); | ||
|
||
// Assume using default locale. |
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.
this comment should be above expectedDescription
too
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.
done
|
||
// Group header DOM roughly matches the clump header DOM, and that's OK. |
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.
should remove this comment. See reasoning in earlier thread, but, more importantly, this wouldn't be the place to put the comment anyways :)
This group header is using group
class names, nothing weird about that or worth commenting on; it's the clump header using group
class names that's the aberration.
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.
done
@@ -113,11 +110,21 @@ | |||
--score-title-line-height-big: 36px; | |||
--score-title-line-height: 26px; | |||
--section-padding: 40px; | |||
--shape-margin: 7px 12px 0 4px; | |||
--shape-size: 12px; |
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.
this would probably be a little more helpful with an additional qualifier in the variable name. Which shape? (same with circle
and square
?)
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.
it's used for both metric icons and audit score icons so I went with --icon-shape-size
...
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.
it's used for both metric icons and audit score icons so I went with
--icon-shape-size
...
"icon" and "shape" mean basically the same thing :)
what about --score-shape-size
, etc?
Codecov Report
@@ Coverage Diff @@
## master #8222 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 283 283
Lines 9534 9529 -5
==========================================
- Hits 8659 8652 -7
- Misses 875 877 +2
Continue to review full report at Codecov.
|
2 similar comments
Codecov Report
@@ Coverage Diff @@
## master #8222 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 283 283
Lines 9534 9529 -5
==========================================
- Hits 8659 8652 -7
- Misses 875 877 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #8222 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 283 283
Lines 9534 9529 -5
==========================================
- Hits 8659 8652 -7
- Misses 875 877 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #8222 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 283 283
Lines 9534 9529 -5
==========================================
- Hits 8659 8652 -7
- Misses 875 877 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #8222 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 283 283
Lines 9534 9529 -5
==========================================
- Hits 8659 8652 -7
- Misses 875 877 +2
Continue to review full report at Codecov.
|
Let's defer the RTL to a follow up PR. Everything else look swell? |
@@ -152,6 +160,7 @@ | |||
--score-title-line-height-big: 26px; | |||
--score-title-line-height: 20px; | |||
--section-padding: 24px; | |||
--shape-margin: 5px 12px 0 2px; |
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.
--score-shape-margin
here too?
Design reference: https://yuinchien.github.io/sandbox/lighthouse/
Some questions that came up:
Will fix tests after comments on design.
#8185