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

Implement google sheets output writer #119

Merged
merged 18 commits into from
Jul 5, 2022
Merged

Implement google sheets output writer #119

merged 18 commits into from
Jul 5, 2022

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Jun 30, 2022

A spreadsheet it made:

https://docs.google.com/spreadsheets/d/1h48etlcEcca1GPPvWZbdFUKZvI-oRU441xVQfusthng/edit#gid=718593030

Try it out!

  • git checkout google-sheets-api
  • npm run build
  • npm link
  • lighthouse-parade ...

Make sure to use the -o google-sheets flag to make it upload to google sheets.

Right now the "sign in with google" oauth thing is in "test mode" so I have to manually enter in the email addresses of test users. When you are going to test this, tell me, so I can add your email there.

Eventually we'll need to verify the app with google which will be kinda annoying but it's necessary to make it open to everyone.

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2022

⚠️ No Changeset found

Latest commit: 9f32ba6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

tsconfig.json Show resolved Hide resolved
@calebeby calebeby marked this pull request as ready for review June 30, 2022 15:02
@calebeby calebeby mentioned this pull request Jun 30, 2022
16 tasks
Copy link
Member

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

Ahh, this is so cool! Left some nits inline but overall this is great!

I wonder if there's some way to let folks know that it may take a minute for the Google Sheet to populate? I was staring at an empty sheet for a bit which might be confusing.

No major feedback from me, but you might want to have others review as well since it's a big change. I assume docs changes come when we merge next into main?

src/cli.ts Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/output-writer/index.ts Show resolved Hide resolved
src/output-writer/google-sheets-writer.ts Outdated Show resolved Hide resolved
src/output-writer/google-sheets-writer.ts Outdated Show resolved Hide resolved
src/output-writer/google-sheets-writer.ts Outdated Show resolved Hide resolved
Co-authored-by: Paul Hebert <[email protected]>
@calebeby
Copy link
Member Author

I assume docs changes come when we merge next into main?

Yes. #117

@calebeby
Copy link
Member Author

@Paul-Hebert ready for another review!

Copy link
Member

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for making those changes Caleb!

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

@Paul-Hebert:

I wonder if there's some way to let folks know that it may take a minute for the Google Sheet to populate? I was staring at an empty sheet for a bit which might be confusing.

Do you have ideas for how we could deal with this? Maybe we could write a row that says "waiting for first report" at the top which gets overridden later?

@Paul-Hebert
Copy link
Member

I think that's a great idea! If we wanted we could also add a note to the CLI output saying "This may take a while" or something, but I think temporary row you suggested makes the most sense 👍

@gerardo-rodriguez
Copy link
Member

A spreadsheet it made:

https://docs.google.com/spreadsheets/d/1h48etlcEcca1GPPvWZbdFUKZvI-oRU441xVQfusthng/edit#gid=718593030

@calebeby I don't know Google Sheets enough to know how hard this is or if it's even possible, but is it possible to lock the top two rows so that when I sort the sheet I don't get the 2nd header row at the bottom? 🤔

Screen Shot 2022-07-01 at 10 48 14 AM

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

Yes, I set up a default filter to use instead of the filter view. If you exit the filter view there will be little icons next to each row header, and you can use those to sort. They will keep the header rows in place.

Unlike a filter view, it does actually modify the order of the data :( but better than nothing I think, and it is nice to have it on the main view instead of requiring people to know where in the menu filter views are.

Screen Shot 2022-07-01 at 10 55 23 AM

@gerardo-rodriguez
Copy link
Member

If you exit the filter view there will be little icons next to each row header, and you can use those to sort. They will keep the header rows in place.

@calebeby I don't see the little icons next to each row header. 🤔

Screen Shot 2022-07-01 at 10 59 55 AM

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

@gerardo-rodriguez You have to have edit access since it actually modifies the order of the data

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

Maybe I can set up a filter view as well as a default filter, so you can make actual edits to the sorting if you have edit access, or just view it sorted differently if you don't have edit access

@gerardo-rodriguez
Copy link
Member

@calebeby If I only run it on one page (say a SPA for example) is it expected that a Performance (category score) of 96 be red? 🤔

Screen Shot 2022-07-01 at 1 46 11 PM

https://docs.google.com/spreadsheets/d/1iv-59IP0jdQRNhejsXDX1yot6kJnNh_eFd3QqFznoy8/edit?usp=sharing

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

@gerardo-rodriguez It is red because it is the lowest value in the column.

You can try editing the conditional formatting settings in the UI to see if there is a way to make it work well with one row as well as multiple.

Another option is for us to apply conditional formatting only after the 2nd URL is finished, but I do not prefer that option.

@Paul-Hebert
Copy link
Member

@gerardo-rodriguez It is red because it is the lowest value in the column.

Oh interesting, I didn't realize that. 🤔 I wonder if it would make more sense to have red, green, and yellow map to "good"/"medium"/"bad" in Lighthouse?

The metrics scores and the perf score are colored according to these ranges:

0 to 49 (red): Poor
50 to 89 (orange): Needs Improvement
90 to 100 (green): Good

I suppose that would only work for the "score" values not the values with units 🤔

@calebeby
Copy link
Member Author

calebeby commented Jul 1, 2022

I suppose that would only work for the "score" values not the values with units 🤔

We already have it conditional based on the type of column, because for "score" columns, higher is better, but for the rest (most of which are in milliseconds), lower is better.

We could probably make that work. But I do like to see a continuous range and have it update based on the range of values, because, e.g., on a site like 11ty.dev which gets 100's across the board, a 98 perf score is a much bigger deal than on cloudfour.com

@gerardo-rodriguez
Copy link
Member

We could probably make that work. But I do like to see a continuous range and have it update based on the range of values, because, e.g., on a site like 11ty.dev which gets 100's across the board, a 98 perf score is a much bigger deal than on cloudfour.com

A continuous range makes sense based on the range of values. But still, even on the 11ty.dev example, it's arguable a 98 perf score should still be in the green range even if that's the low when all of the rest are 100.

If I were taking a screenshot, I'd want to say "Look at all the green!" and not "Look at all the green...(ignore the red, it's actually good also)".

I don't think it should be a blocker to merging this PR and maybe it's okay to have great scores shown as red. Seems like we could create a GH issue and have further discussion. Maybe even from the community as this tool continues to be used. 👍🏽

@Paul-Hebert
Copy link
Member

Yeah, I can see pros and cons to both. I agree it's not a blocker. Maybe in the long run we add a flag to let you choose a formatting option?

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

Watching the Google Sheet get filled in live is super fun.

Excellent work, @calebeby! 🎉

@calebeby calebeby merged commit 32af087 into next Jul 5, 2022
@calebeby calebeby deleted the google-sheets-api branch July 5, 2022 17:24
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.

4 participants