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.

Sign up for GitHub

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

Jump to bottom

Report ui runtime settings #8192

Closed
wants to merge 22 commits into from
Closed

Report ui runtime settings #8192

wants to merge 22 commits into from

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 11, 2019 •
edited
Loading

Branched off of #8121. Will rebase off master when that is merged.

See: https://lh-pr-report-redesign.surge.sh/5

#8185

@connorjclark connorjclark changed the base branch from master to report-ui-gauges April 11, 2019 20:24
@exterkamp
Copy link
Member

url gets weird when it's too long I think.
image

@connorjclark
Copy link
Collaborator Author

url gets weird when it's too long I think.

fixed

exterkamp
exterkamp approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM 🖥<- runtime settings

paulirish
paulirish reviewed Apr 12, 2019
View reviewed changes
lighthouse-core/report/html/report-styles.css
@@ -242,6 +248,10 @@
cursor: pointer;
}

.lh-container {
word-break: break-word;
Copy link
Member

@paulirish paulirish Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

just a warning that i've had word-break challenges in devtools.
there's currently this bit over there: https://cs.chromium.org/chromium/src/third_party/blink/renderer/devtools/front_end/audits2/audits2Panel.css?l=40-42&rcl=312a1d6da6d167ba89c90bd051c5181a66a5184c

let's keep an eye out for how this may trigger issues.

paulirish
paulirish approved these changes Apr 12, 2019
View reviewed changes
lighthouse-core/report/html/templates.html Outdated
}
.lh-env {
padding: var(--default-padding) 0;
}
.lh-env__items {
padding-left: 16px;
margin-bottom: var(--audits-margin-bottom);
Copy link
Member

Choose a reason for hiding this comment

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

redundant.

lighthouse-core/report/html/templates.html Outdated
}
span.lh-env__name {
font-weight: bold;
color: var(--secondary-text-color);
min-width: var(--setting-title-min-width);
flex: .5;
Copy link
Member

Choose a reason for hiding this comment

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

0.5

lighthouse-core/report/html/templates.html Outdated
}
span.lh-env__name {
font-weight: bold;
color: var(--secondary-text-color);
min-width: var(--setting-title-min-width);
Copy link
Member

Choose a reason for hiding this comment

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

let's try to align the names here

env vs setting
name vs title

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@connorjclark connorjclark changed the base branch from report-ui-gauges to master April 13, 2019 00:15
@connorjclark
Copy link
Collaborator Author

i messed up this rebase

@connorjclark connorjclark mentioned this pull request Apr 13, 2019
Merged
@paulirish paulirish mentioned this pull request May 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exterkamp exterkamp exterkamp approved these changes

@paulirish paulirish paulirish approved these changes

@brendankenny brendankenny Awaiting requested review from brendankenny

@patrickhulce patrickhulce Awaiting requested review from patrickhulce

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@connorjclark @exterkamp @googlebot @paulirish @mattzeunert @brendankenny