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

docs: add user flow docs #13134

Merged
merged 4 commits into from
Sep 30, 2021
Merged

docs: add user flow docs #13134

merged 4 commits into from
Sep 30, 2021

Conversation

patrickhulce
Copy link
Collaborator

Summary
Adds some user flow documentation and explainers for the different modes. Feedback welcome :)

Related Issues/PRs
ref #11313

@patrickhulce patrickhulce requested a review from a team as a code owner September 28, 2021 21:30
@patrickhulce patrickhulce requested review from connorjclark and removed request for a team September 28, 2021 21:30
@google-cla google-cla bot added the cla: yes label Sep 28, 2021
@patrickhulce patrickhulce added 9.0 and removed cla: yes labels Sep 28, 2021
@google-cla google-cla bot added the cla: yes label Sep 28, 2021
@google-cla
Copy link

google-cla bot commented Sep 28, 2021

☹️ Sorry, but only Googlers may change the label cla: yes.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

This looks great!

We should add a link to a sample flow report at the top of this page, for motivation.

docs/user-flows.md Outdated Show resolved Hide resolved
async function main() {
const browser = await puppeteer.launch();
const page = await browser.newPage();
const flow = new lighthouse.UserFlow(page);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we expose a newUserFlow? perhaps exposing the api via a class isn't necessary / ideal. idk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @adamraine thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

The ES6 class made the most sense to me, is the benefit here that it matches the puppeteer methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think @connorjclark is referring to the public entrypoint being something non-constructor based, not that we should move away from the class itself.

I have to agree constructor-based public APIs feel gross to me for a node library, but admittedly I don't have a compelling case for why, so I'm fine with either.

Copy link
Member

Choose a reason for hiding this comment

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

Aight I slept on this one and a function entry point SGTM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see #13139

docs/user-flows.md Outdated Show resolved Hide resolved
docs/user-flows.md Outdated Show resolved Hide resolved
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.

4 participants