-
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(auth): running Lighthouse on authenticated pages #9628
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.
Thoughts on the wording of the README tutorial.
I think it is great, makes sense and is easy to follow. I have mainly only small nits for understandability. I think @kaycebasques might be better at that than me though!
Co-Authored-By: Shane Exterkamp <[email protected]>
if we follow the pattern of other docs (which has seemed good but certainly can change), it might make sense to split this into a |
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.
Only had time for a brief review, but in general LGTM. I didn't see any instructional issues with the content. I didn't usability test it, though.
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 great! ❤️
stashed the jest stuff in a branch, will open a new PR with that later. This is ready for final review. |
docs/recipes/auth/server/server.js
Outdated
if (req.session.user) { | ||
res.sendFile('./dashboard.html', {root: PUBLIC_DIR}); | ||
} else { | ||
res.status(401).sendFile('./unauthenticated.html', {root: PUBLIC_DIR}); |
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 this remains a file, it should probably be named dashboard-unauthenticated.html
to distinguish from an auth'd /
login page.
Co-Authored-By: Paul Irish <[email protected]>
Thoughts on using the local Lighthouse code?
plus, adding a sanity test that this works in CI? cd docs/recipes/auth
yarn
yarn concurrently start "node example-lh-auth.js" |
sure. sg.
can you add that in a followup PR? |
@patrickhulce i think you're change requests were all addressed so I am dismissing your review since you are on vacation :) |
https://github.com/GoogleChrome/lighthouse/blob/auth-docs/docs/authenticated-pages.md
https://github.com/GoogleChrome/lighthouse/tree/auth-docs/docs/recipes/auth