Skip to content

Conversation

@adamraine
Copy link
Contributor

Proper grid layout for timespans.

@adamraine adamraine requested a review from a team as a code owner November 4, 2021 23:29
@adamraine adamraine requested review from connorjclark and removed request for a team November 4, 2021 23:29
@google-cla google-cla bot added the cla: yes label Nov 4, 2021
showEl.textContent = Util.i18n.strings.expandView;
hideEl.textContent = Util.i18n.strings.collapseView;

const metricAudits = category.auditRefs.filter(audit => audit.group === 'metrics');
Copy link
Contributor Author

@adamraine adamraine Nov 4, 2021

Choose a reason for hiding this comment

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

Took me a half an hour to figure out why it wasn't working. Turns out we set metricAudits twice.

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 9, 2021
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 thought our plan was to reorder the metrics in default-config and then change the grid css so it scales nicely from 2-6?

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 9, 2021

Having the order in the default be anything other than the order which we deem the metrics to be most important to see visually would be confusing. I prefer fixing it elsewhere.

@brendankenny
Copy link
Contributor

rename the PR since this also affects navigation and classic mode?

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@adamraine adamraine changed the title report(flow): timespan metric grid report: order metrics by row Nov 9, 2021
@brendankenny
Copy link
Contributor

i thought our plan was to reorder the metrics in default-config and then change the grid css so it scales nicely from 2-6?

Having the order in the default be anything other than the order which we deem the metrics to be most important to see visually would be confusing. I prefer fixing it elsewhere.

If in the single column version the order is

  • FCP
  • TTI
  • Speed Index
  • TBT
  • LCP
  • CLS

isn't that what we're deciding is the most important order to see them in visually? If that's the goal, shouldn't we just update the default config to match?

@adamraine
Copy link
Contributor Author

If in the single column version the order is

I hadn't considered the single column version, which doesn't follow the config order anymore. I'm sure we can make a JS solution that preserves the config order AND makes it easy to define the border-bottom for the last two elements AND flows by row for timespan. But It's gonna be more complicated than I would like for what is ultimately just a visual fix.

Is the current config order based on anything, or did it just evolve naturally as metrics were added and removed?

@brendankenny
Copy link
Contributor

Haha, oh I see now, that's why I was so confused by the PR title. This is ultimately to fix that in timespan mode two metrics end up as a column instead of as a row?

There is an intentional order, but @paulirish is the comparator for it so may need to chime in on this :)

@paulirish
Copy link
Member

#13345

@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 9, 2021
@google-cla
Copy link

google-cla bot commented Nov 9, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@adamraine
Copy link
Contributor Author

@paulirish @connorjclark I think yall need to consent to CLA bot

@google-cla
Copy link

google-cla bot commented Nov 10, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 10, 2021
@paulirish
Copy link
Member

@googlebot I consent

@google-cla
Copy link

google-cla bot commented Nov 10, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@paulirish paulirish merged commit fcab9cf into master Nov 10, 2021
@paulirish paulirish deleted the flow-timespan-metrics branch November 10, 2021 20:50
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.

5 participants