-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(user-flows): refactor document #14021
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the images to the assets folder? They're not the same images used in the doc.
|
||
```js | ||
import {writeFileSync} from 'fs'; | ||
import puppeteer from 'puppeteer'; | ||
import api from 'lighthouse/lighthouse-core/fraggle-rock/api.js'; | ||
import lighthouse from 'lighthouse/lighthouse-core/fraggle-rock/api.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally changed this to api
because it's not the same as doing import lighthouse from 'lighthouse'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes sense. but i also think api
is too general. it looks 'fine' on this line but then later with api.startFlow
it's lost its identity
lighthouseapi
? lhapi
?
we should also fix the export so that import lighthouse from 'lighthouse'
actually would work for all this. i feel good about doing that for v10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also fix the export so that import lighthouse from 'lighthouse' actually would work for all this. i feel good about doing that for v10
Ok, in that case we can leave this with plans to update the docs once startFlow
is exported via the default entry point.
docs/user-flows.md
Outdated
Navigation reports analyze a single page load. Navigation is the most common type of report you'll see. In fact, all Lighthouse reports prior to v9 are navigation reports. | ||
| | | | ||
|---|---| | ||
| Navigation | <small> *Benefits* <br> ✅ Provides an overall performance score and all metrics.<br>✅ Contains the most advice of all report types (both time-based and state-based audits are available).<br> *Limitations* <br> 🤔 Cannot analyze form submissions or single page app transitions.<br>🤔 Cannot analyze content that isn't available immediately on page load.<br> *Use Cases* <br> 👍 Obtain a Lighthouse Performance score.<br>👍 Measure Performance metrics (First Contentful Paint, Largest Contentful Paint, Speed Index, Time to Interactive, Cumulative Layout Shift, Total Blocking Time).<br>👍 Assess Progressive Web App capabilities.</small> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use a table here, can benefits/limitations/use cases each have there own column? If that doesn't look good then maybe we're better off using headings/subheadings for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Adam Raine <[email protected]>
Co-authored-by: Adam Raine <[email protected]>
true. I just wanted to have them in a more accessible place than locked up in figma. |
docs/user-flows.md
Outdated
### Navigation | ||
| | | | ||
|:---:|---| | ||
| Navigation <br> <img src="https://user-images.githubusercontent.com/39191/168928225-f2157fda-5131-4bd0-9121-b1a0b2f869a7.png" height="96" width="141"> | **Benefits** <br> ✅ Provides an overall performance score and all metrics.<br>✅ Contains the most advice of all report types.<br><br> **Limitations** <br> 🤔 Cannot analyze form submissions or single page app transitions.<br>🤔 Cannot analyze content that isn't available immediately on page load.<br><br> **Use Cases** <br> 👍 Obtain a Lighthouse Performance score.<br>👍 Measure all performance metrics.<br>👍 Assess Progressive Web App capabilities. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add these images to the more detailed sections below this table so there is a god visual callout. I was thinking something similar to the "Creating a Flow" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. done
docs/user-flows.md
Outdated
|:---:|---| | ||
| Navigation <br> <img src="https://user-images.githubusercontent.com/39191/168928225-f2157fda-5131-4bd0-9121-b1a0b2f869a7.png" height="96" width="141"> | **Benefits** <br> ✅ Provides an overall performance score and all metrics.<br>✅ Contains the most advice of all report types.<br><br> **Limitations** <br> 🤔 Cannot analyze form submissions or single page app transitions.<br>🤔 Cannot analyze content that isn't available immediately on page load.<br><br> **Use Cases** <br> 👍 Obtain a Lighthouse Performance score.<br>👍 Measure all performance metrics.<br>👍 Assess Progressive Web App capabilities. | | ||
| Timespan <br> <img src="https://user-images.githubusercontent.com/39191/168928251-c7025cd5-0086-4db8-ae52-95a5b5675adf.png" height="96" width="141"> | **Benefits** <br> ✅ Provides timerange-based metrics such as TBT, CLS and INP.<br>✅ Analyzes any period of time, including user interactions or single page app transitions.<br><br> **Limitations** <br> 🤔 Does not provide an overall performance score.<br>🤔 Cannot analyze moment-based performance metrics (e.g. Largest Contentful Paint).<br>🤔 Cannot analyze state-of-the-page issues (e.g. no Accessibility category)<br><br> **Use Cases** <br> 👍 Measure layout shifts and JavaScript execution time on a series of interactions.<br>👍 Discover performance opportunities to improve the experience for long-lived pages and SPAs. | | ||
| Snapshot <br> <img src="https://user-images.githubusercontent.com/39191/168931653-b45e0b6b-c5bd-4d8d-85b6-fee8425186a0.png" height="96" width="141"> | **Benefits** <br> ✅ Analyzes the page in its current state.<br><br> **Limitations** <br> 🤔 Does not provide an overall performance score or metrics.<br>🤔 Cannot analyze any issues outside the current DOM (e.g. no network, main-thread, or performance analysis).<br><br> **Use Cases** <br> 👍 Find accessibility issues in single page applications or complex forms.<br>👍 Evaluate best practices of menus and UI elements hidden behind interaction. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda hard to read/review from the md side. Is it possible to use newlines to separate each bullet point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately i can't. but the text is the same as what it was. (except i shortened a few metric references)
docs/user-flows.md
Outdated
So far we've seen individual Lighthouse modes in action. The true power of flows comes from combining these building blocks into a comprehensive flow to capture the user's entire experience. | ||
|
||
### Selecting Boundaries | ||
<img src="https://user-images.githubusercontent.com/39191/168931109-5ae80620-7464-4d98-a834-71169a5b6a61.png" height=300> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this guy be a little smaller, maybe 200px?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matches the rest now. 240px
docs/user-flows.md
Outdated
### Navigation | ||
| | | | ||
|:---:|---| | ||
| Navigation <br> <img src="https://user-images.githubusercontent.com/39191/168928225-f2157fda-5131-4bd0-9121-b1a0b2f869a7.png" height="96" width="141"> | **Benefits** <br> ✅ Provides an overall performance score and all metrics.<br>✅ Contains the most advice of all report types.<br><br> **Limitations** <br> 🤔 Cannot analyze form submissions or single page app transitions.<br>🤔 Cannot analyze content that isn't available immediately on page load.<br><br> **Use Cases** <br> 👍 Obtain a Lighthouse Performance score.<br>👍 Measure all performance metrics.<br>👍 Assess Progressive Web App capabilities. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need a "Benefits" and and "Use Cases" section for each mode. A lot of the points are repeated between the two sections. WDYT about just having a "Use Cases" and "Limitations" section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i love it. i'm always a fan of brevity. i merged those two and it looks a lot better now.
|
||
```js | ||
import {writeFileSync} from 'fs'; | ||
import puppeteer from 'puppeteer'; | ||
import api from 'lighthouse/lighthouse-core/fraggle-rock/api.js'; | ||
import lighthouse from 'lighthouse/lighthouse-core/fraggle-rock/api.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also fix the export so that import lighthouse from 'lighthouse' actually would work for all this. i feel good about doing that for v10
Ok, in that case we can leave this with plans to update the docs once startFlow
is exported via the default entry point.
Co-authored-by: Adam Raine <[email protected]>
6ea4f5a
to
b7f5c68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good
Co-authored-by: Adam Raine <[email protected]>
docs/user-flows.md
Outdated
@@ -14,7 +14,7 @@ In these cases, you want Lighthouse on a _flow_, not just a page load. | |||
|
|||
Lighthouse can now run in three modes: navigations, timespans, and snapshots. Each mode has its own unique use cases, benefits, and limitations. Later, you'll create a flow by combining these three core report types. | |||
|
|||
* **Navigation mode** analyzes a single page load. Prior to v9.6.1, all Lighthouse runs were essentially in this mode. | |||
* **Navigation mode** analyzes a single page load. Prior to v9.6.0, all Lighthouse runs were essentially in this mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reword? I can't tell at a glance without employing multiple brain cells if this is exclusive or not :') "Starting in..." would work better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about
- Navigation mode analyzes a single page load. It should feel familiar as all Lighthouse runs prior to v9.6.0 were essentially in this mode. Even in v9.6.0 and onwards, it remains the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, some last minute thoughts
nit: |
a lot changed. i added a summary to the PR description. don't know what else to call it, but feel free to edit the title. |
building on adam's great work in #14009 I wanted to try to frontload the mode descriptions so devtools folks understand these concepts without having to see the nodejs api.
Changes:
I'm pretty happy with how it stands now. decent job with both audiences: 1) devtools folks clicking through on "wtf is this mode selector" and 2) folks wanting to properly analyze userflows with the js api
preview: https://github.com/GoogleChrome/lighthouse/blob/dt-user-flows-doc-me/docs/user-flows.md