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(redesign): inline display text. bold audit header text #8592

Merged
merged 7 commits into from
May 3, 2019
Merged

report(redesign): inline display text. bold audit header text #8592

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a0964bb
report(redesign): inline display text. bold audit header text.
connorjclark Apr 24, 2019
b2b3bc3
force travis
connorjclark Apr 25, 2019
1e1d083
Update lighthouse-core/report/html/report-styles.css
paulirish Apr 29, 2019
7720998
Merge remote-tracking branch 'origin/master' into rd-audit-title
connorjclark Apr 29, 2019
269f6c8
spacing
connorjclark Apr 29, 2019
afe4540
fix selector for bp
connorjclark Apr 29, 2019
fe24d28
Merge remote-tracking branch 'origin/master' into rd-audit-title
connorjclark May 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lighthouse-core/report/html/report-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,6 @@
}


.lh-audit__title,
.lh-audit__display-text,
.lh-load-opportunity__sparkline,
.lh-chevron-container {
Expand All @@ -453,13 +452,20 @@
margin-right: 0;
}

.lh-audit__title {
.lh-audit__display-text {
flex: 1;
}

/* Prepend display text with em dash separator. But not in Opportunities. */
.lh-audit-group:not(.lh-audit-group--load-opportunities) .lh-audit__display-text:not(:empty):before {
Copy link
Member

Choose a reason for hiding this comment

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

while this selector does look crazy, it turns out to be reasonable.. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:)

Copy link
Member

Choose a reason for hiding this comment

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

best practices aren't grouped so aren't getting the :before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

content: '—';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
content: '—';
content: '\2014';

Copy link
Member

Choose a reason for hiding this comment

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

can this just go in the HTML? Kind of weird to do it with a pseudo-element

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why? dt encoding issues are resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brendankenny can't just be HTML, need some sort of logic check. makes sense to me to have the display + logic in one place here.

Copy link
Member

Choose a reason for hiding this comment

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

can't just be HTML, need some sort of logic check. makes sense to me to have the display + logic in one place here.

I don't think it needs to be. Opportunities and the rest of the audits share text with a .lh-audit__display-text class, but they actually come from different templates, #tmpl-lh-opportunity vs #tmpl-lh-audit. If the #tmpl-lh-audit template gets the em dash inline, then it'll only show up there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch on the template thing. But would still need a length check - and can't make a CSS rule for preceding sibling (pseudo code below).

image

.lh-audit__separator {
  margin: 0 var(--audit-item-gap);
}
/* not actually a thing */
.lh-audit__display-text:not(:empty) - .lh-audit__separator {
  display: none;
}

Copy link
Member

Choose a reason for hiding this comment

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

per discussion in chat.. let's use whitespace on both sides of the dash.

Assuming we want to use var(--audit-item-gap) the we'll need some margin in here too:

  margin-right: var(--audit-item-gap);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

margin-right: var(--audit-item-gap);
}

/* Expandable Details (Audit Groups, Audits) */
.lh-audit__header {
display: flex;
font-weight: 500;
padding: var(--lh-audit-vpadding) 0;
}

Expand Down Expand Up @@ -622,7 +628,6 @@

.lh-load-opportunity__col {
display: flex;
justify-content: space-between;
}

.lh-load-opportunity__col--one {
Expand Down Expand Up @@ -806,6 +811,7 @@
}
.lh-audit-group__header span.lh-audit-group__description::before {
content: '—';
margin: 0px var(--audit-item-gap);
}

.lh-clump > .lh-audit-group__header,
Expand Down