Skip to content
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: make denser. changes to typography, group descriptions, more #13249

Merged
merged 35 commits into from Nov 2, 2021

Conversation

paulirish
Copy link
Member

Quite a few changes in here. I've been iterating with jiwoong over the past week.

Currently draft, but will probably flip in a few.

@google-cla google-cla bot added the cla: yes label Oct 21, 2021
@adamraine
Copy link
Member

Some issues with category fractions:

Screen Shot 2021-10-26 at 10 44 33 AM

@adamraine
Copy link
Member

adamraine commented Oct 26, 2021

Edit: Looks like this is an existing issue, I'll look into it.

We still want these metrics to be full width right?

Screen Shot 2021-10-26 at 12 19 54 PM

--color-teal-600: #00897B;
--color-white: #FFFFFF;

/* Context-specific colors */
--color-average-secondary: var(--color-orange-700);
--color-average: var(--color-orange);
--color-average-text: #C33300;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--color-orange-700 instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jiwoong selected this instead.

@paulirish paulirish added the 9.0 label Nov 1, 2021
@paulirish
Copy link
Member Author

Average gauge text looks really red in dark mode:

Yup. Known issue.

@adamraine
Copy link
Member

adamraine commented Nov 1, 2021

Moa stuff I found

Some links are missing the correct color:

Screen Shot 2021-11-01 at 6 13 45 PM

Also warning text has color contrast issues

Screen Shot 2021-11-01 at 6 35 29 PM

report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
@@ -1025,14 +975,15 @@
color: var(--color-red-700);
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change

report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved


details .lh-clump-toggle::before {content: 'Show';}
details[open] .lh-clump-toggle::before {content: 'Hide';}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more strings that could be localized here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulled into separate PR #13308

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM

@@ -1820,20 +1786,23 @@


.lh-meta__items {
--meta-icon-size: calc(var(--report-icon-size) * 0.667);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be full width? I kinda liked it when it was narrower + centered.

@@ -1153,9 +1081,13 @@

.lh-warnings {
--item-margin: calc(var(--report-line-height) / 6);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also takes up the full width where it didn't before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants