-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add Total Blocking Time documentation for Lighthouse #1636
Conversation
Deploy preview for web-dev-staging ready! Built with commit eb0dbb7 |
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
Hello @patrickhulce and @deepanjanroy, I wrote documentation for Total Blocking Time. This will be the page that the Lighthouse report UI links to. Please provide a technical review of the content:
|
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.
looks pretty good to me other than the minor technicality!
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
@patrickhulce thank you for the speedy review, please go through your previous comments and resolve them if you think the updated content addresses your feedback |
My resolve comment button seems to be missing on this repo, but everything looks addressed to me! |
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.
lgtm!
src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Outdated
Show resolved
Hide resolved
Thank you @patrickhulce and @deepanjanroy! |
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.
LGTM.
I guess one thing to think about is how we want to handle all of these URLs when Phil does his metrics section because I imagine he'll also want to talk about Total Blocking Time.
@robdodson good point. I renamed it to |
I reviewed this with a fresh pair of eyes. Since Rob also reviewed, as well as Patrick (who's usually pretty thorough even though he's only responsible for a technical review) the content should be fine, so I'm going to merge. |
Another option would be to have Phil's guides use something like |
Fixes #1627
Changes proposed in this pull request:
/src/site/content/en/lighthouse-performance/total-blocking-time/index.md
Staged page: https://deploy-preview-1636--web-dev-staging.netlify.com/total-blocking-time/