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

handle v7 #31

Merged
merged 4 commits into from
Dec 22, 2020
Merged

handle v7 #31

merged 4 commits into from
Dec 22, 2020

Conversation

paulirish
Copy link
Owner

  • version=7 in the URL now works
    • example: #FCP=671&SI=671&LCP=762&TTI=671&TBT=0&CLS=0&FCI=671&FMP=671&device=mobile&version=7.0.0
  • default UI now excludes v5 scores
  • UX-wise, to help communicate that v6 and v7 scoring is identical, i've grouped them into a single "item"

@@ -49,6 +49,11 @@ function makeScoringGuide(curves) {
}

export const scoringGuides = {
// v7 scoring is identical to v6
v7: {
Copy link
Owner Author

Choose a reason for hiding this comment

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

given the version collapsing, this bit isn't necessary, but it feels good for anyone else consuming this code.

script/main.js Outdated Show resolved Hide resolved
script/main.js Outdated
render() {
const {versions, device, metricValues} = this.state;

this.collapseVersions(versions);
Copy link
Collaborator

@connorjclark connorjclark Dec 21, 2020

Choose a reason for hiding this comment

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

Does this have to mutate state? Can it return a new array and be used on L281?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ill let you take another look as i changed things quite a bit.

Co-authored-by: Connor Clark <[email protected]>
script/main.js Outdated
@@ -178,7 +178,7 @@ class ScoringGuide extends Component {

let title = <h2>{name}</h2>;
if (name === 'v6') {
title = <h2><a href="https://github.com/GoogleChrome/lighthouse/releases/tag/v6.0.0" target="_blank">v6</a></h2>;
title = <h2>latest<br/><i>v6, v7</i></h2>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

could just link to the releases page?

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

throw new Error(`Unsupported Lighthouse version (${version})`);
}
// In the future we might want a more generalized `version % 2 === 1` thing, but for now, hardcode the change.
if (parseInt(version) === 7) return 6..toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

hello ruby my old friend

Choose a reason for hiding this comment

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

Whoa range operator

Copy link
Owner Author

Choose a reason for hiding this comment

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

😀 not range operator. it's just how you call methods on a number literal.

one could do 123.45.toLocaleString() but if you're calling on an integer, you'll need two periods: 123..toLocaleString(). (If you try 123.toLocaleString(), you'll get a SyntaxError.

i dont remember where i picked this up, but it's some esoteric JS knowledge that's been tucked away in my brain. :p

// In the future we might want a more generalized `version % 2 === 1` thing, but for now, hardcode the change.
if (parseInt(version) === 7) return 6..toString();
return version.toString();
}).sort().reverse();
Copy link
Collaborator

@connorjclark connorjclark Dec 22, 2020

Choose a reason for hiding this comment

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

default sort sorts as strings...idk problem for future us or wanna knock it out now?

Copy link
Owner Author

Choose a reason for hiding this comment

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

shouldnt matter tbh. leaving for later if it ever does matter :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

BOOKMARKED

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

Successfully merging this pull request may close these issues.

3 participants