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

misc: check doc page by Lighthouse CI #7300

Merged
merged 4 commits into from
May 4, 2022
Merged

misc: check doc page by Lighthouse CI #7300

merged 4 commits into from
May 4, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented May 4, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Since the doc page represents the main functionality of Docusaurus, Lighthouse should check it instead of the landing page. For example, the a11y error with the home page in the breadcrumbs could have been detected earlier.

@lex111 lex111 added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label May 4, 2022
@lex111 lex111 requested review from slorber and Josh-Cena as code owners May 4, 2022 06:51
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 4, 2022
@netlify
Copy link

netlify bot commented May 4, 2022

[V2]

Name Link
🔨 Latest commit 2a2396f
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62726dd764e02200087ef57a
😎 Deploy Preview https://deploy-preview-7300--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 4, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 59
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-7300--docusaurus-2.netlify.app/

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Can we check both?

The landing page's performance is still important, like FCP

@Josh-Cena
Copy link
Collaborator

Why does it always only check one URL😢 When you check the actions log for 3a5d4d4 and ea22edd, they seem to still stick with the old config

@lex111
Copy link
Contributor Author

lex111 commented May 4, 2022

Perhaps need to merge this change so that the config for CI is updated?

@Josh-Cena
Copy link
Collaborator

Theoretically, the action should be using the config on this branch, especially since this is not in a fork. At least that's how I tested my config changes. My main worry is how audits on multiple URLs will be formatted, so I'd need to see at least what the output looks like.

@Josh-Cena
Copy link
Collaborator

@lex111 From our actions/github-script, we only take the first check's manifest and post it as comment. Can you add the second one's as well? We can simply add another column to the table.

@lex111
Copy link
Contributor Author

lex111 commented May 4, 2022

I don't quite understand you, it will be for each of the checked pages by a separate table?

@Josh-Cena
Copy link
Collaborator

I'm suggesting it to be

| Category | Landing page score | Doc page score |
| --- | --- | --- |
| Performance | 🟠 57 | 🟠 57 |
| Accessibility | 🟢 100 | 🟢 100 |
| Best practices | 🟢 92 | 🟢 92 |
| SEO | 🟢 100 | 🟢 100 |
| PWA | 🟢 90 | 🟢 90 |

So instead of using [0] index, we just use a map instead.

@lex111
Copy link
Contributor Author

lex111 commented May 4, 2022

I have another idea how visualize data that should work, but it is problematic to test these changes.

@lex111 lex111 force-pushed the lex111-patch-1 branch from 975102b to 2a2396f Compare May 4, 2022 12:13
@Josh-Cena
Copy link
Collaborator

Yeah, I can understand. Do you want to merge this so we can test it?

@lex111
Copy link
Contributor Author

lex111 commented May 4, 2022

Yes, let's check it out in action.

@lex111 lex111 merged commit ee20387 into main May 4, 2022
@lex111 lex111 deleted the lex111-patch-1 branch May 4, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants