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: use css grid for metrics #9273

Merged
merged 5 commits into from Jun 28, 2019
Merged

report: use css grid for metrics #9273

merged 5 commits into from Jun 28, 2019

Conversation

connorjclark
Copy link
Collaborator

Before:
See #9270

After:

image
image
image
image

Note, I merged #9272 into this changeset b/c the grid styles here require that the score icon is always present and visible.

@@ -570,10 +566,9 @@
}

.lh-metric__innerwrap {
display: flex;
display: grid;
grid-template-columns: 1fr 80% 20%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait what's the need for 3 columns, aren't we just using two?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also what is the value of 1fr when we've supposedly already used up 100% of the available width...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the first column is the score icon

1fr is a nice way to say "give this some minimum space" but still assigned the 2nd and 3rd columns with percentages that add up to 100

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

i was definitely hoping it would be that easy.. alas...

with long content it will overhang the parent container width.
image

i think something more like this will work better:
grid-template-columns: var(--audit-description-padding-left) 10fr 3fr;

separately... the metric value should be right aligned (we got it for free in flex land)
image

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

updating PR status

@connorjclark
Copy link
Collaborator Author

@paulirish bump

@@ -593,13 +588,15 @@

.lh-metric__description {
display: none;
grid-column-start: 1;
grid-column-end: 3;
Copy link
Member

Choose a reason for hiding this comment

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

looking at https://yuinchien.com/sandbox/lighthouse/
image

it seems this is currently misaligned...
image

perhaps:

  grid-column-start: 2;
  grid-column-end: 4;

? i dont really know grid though... can you doublecheck?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya that did the trick

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

Successfully merging this pull request may close these issues.

None yet

4 participants