Skip to content

Conversation

@Mirzamohammad22
Copy link
Contributor

On using the package i noticed some features could be slightly improved.

Issues:

  1. Not being able to run both emulationFormFactors in the same call and see pr comments of both without replication of comments.
  2. Pr comments not having information about which emulationFormFactor it ran on, anyone reviewing the pr would have to see the full report for it.

This Pr is meant to add these improvements:

  1. Add a new 'both' emulationFormFactor for local lighthouse to run 'mobile' and 'desktop'
  2. Add the 'mode' for pr comments so reviewers have more context to work with, without having to see the full report.

@Mirzamohammad22
Copy link
Contributor Author

Mirzamohammad22 commented Jun 24, 2020

Hey @adamhenson .Awsome project, just some minor contributions based on experience on my end. Would love to have your suggestions on it.

@adamhenson
Copy link
Collaborator

Thanks for opening this @Mirzamohammad22. I think this could be a beneficial feature. I left a few comments. I'll work on issue #33 so we can test it.

@Mirzamohammad22
Copy link
Contributor Author

@adamhenson Would love to have these improvements in your lighthouse check action.

@adamhenson
Copy link
Collaborator

Thanks for making these changes @Mirzamohammad22 - I just added a couple last ones. I think we should remove lighthouseCaller because we don't need the console log inside of it. Since we added the default emulatedFormFactor = 'mobile', the console log outside of that function works as is 👍. If I'm not making sense - I could create a PR into your branch maybe.

@adamhenson
Copy link
Collaborator

Yep, once we get this merged, I'll publish a new version and bump the GitHub Action to consume it.

@Mirzamohammad22
Copy link
Contributor Author

Mirzamohammad22 commented Jun 25, 2020

Thanks for making these changes @Mirzamohammad22 - I just added a couple last ones. I think we should remove lighthouseCaller because we don't need the console log inside of it. Since we added the default emulatedFormFactor = 'mobile', the console log outside of that function works as is 👍. If I'm not making sense - I could create a PR into your branch maybe.
I'll make the changes, no issues.

auditResults.push(lighthouseAuditResult);
index++;
if (options.emulatedFormFactor !== 'both') {
if (verbose) {
Copy link
Collaborator

@adamhenson adamhenson Jun 25, 2020

Choose a reason for hiding this comment

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

Sorry if I wasn't clear @Mirzamohammad22. My point is that we don't need to have this block repeated now. We can leave it where it was.

EDITED: My mistake, I see why we need it, because options is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issues , let me know if the If case is fine or should i revert to lighthouseCaller

@adamhenson adamhenson changed the base branch from master to feat/enhanced-commenting June 25, 2020 17:30
@adamhenson
Copy link
Collaborator

@Mirzamohammad22 thanks again for your work on this. I'm merging this into another branch I created, so I can make some minor changes. I'll let you know when I've merged and published it (should be EOD).

@adamhenson adamhenson merged commit 487b47e into foo-software:feat/enhanced-commenting Jun 25, 2020
@Mirzamohammad22
Copy link
Contributor Author

@Mirzamohammad22 thanks again for your work on this. I'm merging this into another branch I created, so I can make some minor changes. I'll let you know when I've merged and published it (should be EOD).

Awesome.Thanks for the approval , looking forward to having this feature in GitHub actions!

@adamhenson
Copy link
Collaborator

@Mirzamohammad22 this has been released added to the GitHub Action in v2.0.1 🎈 - thanks again!

@adamhenson
Copy link
Collaborator

adamhenson commented Jun 25, 2020

Note - I updated emulatedFormFactor to accept all vs both.

@Mirzamohammad22
Copy link
Contributor Author

@Mirzamohammad22 this has been released added to the GitHub Action in v2.0.1 🎈 - thanks again!

Thank you!

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.

2 participants