feat(server): add compare different url #457

pndllxzzy · 2020-09-23T13:28:19Z

No description provided.

googlebot · 2020-09-23T13:28:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

pndllxzzy · 2020-09-23T13:31:18Z

@googlebot I signed it!

googlebot · 2020-09-23T13:31:26Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

patrickhulce · 2020-09-23T14:29:29Z

thanks for the PR @pndllxzzy! It gets a little crowded by sticking it up there though and the click target moves when you hover because of "Open Report" :/

Other ideas:

hide the base and compare URL selectors behind the menu where you select the build/hash
have both dropdowns in the original location of the compare URL dropdown (add more labels to differentiate)

pndllxzzy · 2020-09-25T16:06:43Z

@patrickhulce Thanks for your ideas.Do you mean like this?

patrickhulce · 2020-09-25T16:11:02Z

Yeah, that seems to get the job done even if it's not the prettiest (not referring to your implementation being not pretty, I know it was my suggestion 😄 ). WDYT?

googlebot · 2020-09-25T17:12:10Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

pndllxzzy · 2020-09-25T17:21:33Z

@patrickhulce I agree with you. I'm not good at beautiful layout. If you have any ideas, please tell me.🤝🏻

patrickhulce

thanks @pndllxzzy this is great! I think you might need to update some snapshot tests as well?

patrickhulce · 2020-09-25T18:03:53Z

packages/server/src/ui/routes/build-view/lhr-comparison.jsx

@@ -113,13 +113,14 @@ export function computeAuditGroups(lhr, baseLhr, options) {
  return auditGroups;
 }

-/** @param {{hookElements: LHCI.HookElements<'dropdowns'>, selectedAuditId?: string | null, lhr?: LH.Result, baseLhr?: LH.Result, percentAbsoluteDeltaThreshold: number, setPercentAbsoluteDeltaThreshold: (x: number) => void}} props */


I think we can leave this just as dropdowns it was already set to be plural for more than one.

use a fragment when it's used instead?

Do you mean like this?

<LhrComparison ... hookElements={{ ... dropdowns: ( <Fragment> <Dropdown label="Base Url" className="dropdown--url dropdown--base-url" value={baseUrl} setValue={url => { const to = new URL(window.location.href); to.searchParams.set('baseUrl', url); to.searchParams.set('compareUrl', compareUrl); route(`${to.pathname}${to.search}`); }} options={availableUrlOptions} /> <Dropdown label="Compare Url" className="dropdown--url dropdown--compare-url" value={compareUrl} setValue={url => { const to = new URL(window.location.href); to.searchParams.set('baseUrl', baseUrl); to.searchParams.set('compareUrl', url); route(`${to.pathname}${to.search}`); }} options={availableUrlOptions} /> </Fragment> ), }} />

patrickhulce · 2020-09-25T18:04:39Z

packages/server/src/ui/routes/build-view/build-view.jsx

@@ -126,7 +130,7 @@ const BuildView_ = props => {
            lhr={lhr}
            isDimmed={openBuildHash === 'base'}
            isOpen={openBuildHash === 'compare'}
-            setLhrLinkHover={setLhrLinkHover}
+            setLhrLinkHover={setLhrCompareLinkHover}


nice split 👍

patrickhulce · 2020-09-25T18:05:25Z

packages/server/src/ui/routes/build-view/build-view.jsx

-          dropdowns: (
+          dropdownsBase: (
+            <Dropdown
+              label="BaseUrl"


Suggested change

label="BaseUrl"

label="Base URL"

patrickhulce · 2020-09-25T18:05:44Z

packages/server/src/ui/routes/build-view/build-view.jsx

-              label="URL"
-              className="dropdown--url"
-              value={selectedUrl}
+              label="CompareUrl"


Suggested change

label="CompareUrl"

label="Compare URL"

pndllxzzy · 2020-10-09T15:45:24Z

@patrickhulce How to update snapshot?
Use this command?
jest --updateSnapshot

patrickhulce · 2020-10-09T16:06:03Z

Yep! I tend to use -u but that works too :)

patrickhulce

just these two leftover from previous review but other than that looks good with test fixes! :) thanks for doing this @pndllxzzy

packages/server/src/ui/routes/build-view/build-view.jsx

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

pndllxzzy · 2020-10-11T12:43:04Z

@patrickhulce Should I rebase all commits to one?

patrickhulce · 2020-10-11T15:01:03Z

Should I rebase all commits to one?

Don't worry about it we'll squash when we merge :) I'll take a closer look here tomorrow 👍

patrickhulce

thanks for sticking with this @pndllxzzy! 🎉

pndllxzzy force-pushed the master branch from 33ce4ee to ce29169 Compare September 25, 2020 17:07

feat(server): add compare different url

a1f9c36

pndllxzzy force-pushed the master branch from ce29169 to a1f9c36 Compare September 25, 2020 17:12

patrickhulce reviewed Sep 25, 2020

View reviewed changes

feat(server): combine dropdowns

6072235

patrickhulce requested changes Oct 9, 2020

View reviewed changes

packages/server/src/ui/routes/build-view/build-view.jsx Outdated Show resolved Hide resolved

packages/server/src/ui/routes/build-view/build-view.jsx Outdated Show resolved Hide resolved

pndllxzzy and others added 2 commits October 11, 2020 20:06

Update packages/server/src/ui/routes/build-view/build-view.jsx

bf2b33f

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

Update packages/server/src/ui/routes/build-view/build-view.jsx

d91bf2c

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

patrickhulce approved these changes Oct 12, 2020

View reviewed changes

patrickhulce merged commit 01e397b into GoogleChrome:master Oct 12, 2020

denkrasnov pushed a commit to Travix-International/lighthouse-ci that referenced this pull request Oct 29, 2020

feat(server): add compare URL dropdown (GoogleChrome#457)

0ba3ec1

This was referenced Dec 19, 2020

[Snyk] Upgrade @lhci/utils from 0.1.0 to 0.6.0 XirdigH/lighthouse-ci#1

Open

[Snyk] Upgrade @lhci/server from 0.4.4 to 0.6.0 XirdigH/lighthouse-ci#3

Open

This was referenced Jan 28, 2021

[Snyk] Upgrade @lhci/utils from 0.1.0 to 0.6.0 Qdigital/lighthouse-ci#2

Open

[Snyk] Upgrade @lhci/server from 0.4.4 to 0.6.0 Qdigital/lighthouse-ci#4

Open

This was referenced Mar 3, 2021

[Snyk] Upgrade @lhci/utils from 0.1.0 to 0.6.1 Travix-International/lighthouse-ci#2

Closed

[Snyk] Upgrade @lhci/server from 0.4.4 to 0.6.1 Travix-International/lighthouse-ci#5

Closed

snyk-bot mentioned this pull request Oct 11, 2021

[Snyk] Security upgrade @lhci/cli from 0.3.9 to 0.5.1 thexdesk/lighthouse-ci#3

Open

snyk-bot mentioned this pull request Mar 23, 2022

[Snyk] Security upgrade @lhci/cli from 0.3.9 to 0.5.1 thexdesk/lighthouse-ci#12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): add compare different url #457

feat(server): add compare different url #457

pndllxzzy commented Sep 23, 2020

googlebot commented Sep 23, 2020

pndllxzzy commented Sep 23, 2020

googlebot commented Sep 23, 2020

patrickhulce commented Sep 23, 2020

pndllxzzy commented Sep 25, 2020 •

edited

patrickhulce commented Sep 25, 2020 •

edited

googlebot commented Sep 25, 2020

pndllxzzy commented Sep 25, 2020

patrickhulce left a comment

patrickhulce Sep 25, 2020

pndllxzzy Oct 9, 2020

patrickhulce Sep 25, 2020

patrickhulce Sep 25, 2020

patrickhulce Sep 25, 2020

pndllxzzy commented Oct 9, 2020

patrickhulce commented Oct 9, 2020

patrickhulce left a comment

pndllxzzy commented Oct 11, 2020

patrickhulce commented Oct 11, 2020

patrickhulce left a comment

feat(server): add compare different url #457

feat(server): add compare different url #457

Conversation

pndllxzzy commented Sep 23, 2020

googlebot commented Sep 23, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

pndllxzzy commented Sep 23, 2020

googlebot commented Sep 23, 2020

patrickhulce commented Sep 23, 2020

pndllxzzy commented Sep 25, 2020 • edited

patrickhulce commented Sep 25, 2020 • edited

googlebot commented Sep 25, 2020

pndllxzzy commented Sep 25, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Sep 25, 2020

Choose a reason for hiding this comment

pndllxzzy Oct 9, 2020

Choose a reason for hiding this comment

patrickhulce Sep 25, 2020

Choose a reason for hiding this comment

patrickhulce Sep 25, 2020

Choose a reason for hiding this comment

patrickhulce Sep 25, 2020

Choose a reason for hiding this comment

pndllxzzy commented Oct 9, 2020

patrickhulce commented Oct 9, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

pndllxzzy commented Oct 11, 2020

patrickhulce commented Oct 11, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

pndllxzzy commented Sep 25, 2020 •

edited

patrickhulce commented Sep 25, 2020 •

edited