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

Fix errors reported by Lighthouse CI #661

Closed
3 of 7 tasks
coderbyheart opened this issue Sep 17, 2022 · 18 comments
Closed
3 of 7 tasks

Fix errors reported by Lighthouse CI #661

coderbyheart opened this issue Sep 17, 2022 · 18 comments
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest help wanted Extra attention is needed

Comments

@coderbyheart
Copy link
Member

coderbyheart commented Sep 17, 2022

#659 will enable Lighthouse CI for every build, however it will not mark a PR as failed because we first need to either fix the currently reported issues, or adapt the rules to ignore this kind of error.

You can also run Lighthouse yourself using Google Chrome:

  1. build the website
  2. run npx node-static public
  3. open an Icognito Window in Google Chrome (this will have you use a blank cache and disable all plugins)
  4. navigate to https://github.com/GoogleChrome/lighthouse-ci
  5. open the developer console (F12) and select the "Lighthouse" tab
  6. Click "Analyze page load" to create a report

There are many errors on all pages, not just on the start page, so feel free to explore them. Please reference this issue in your PR.

Fixing the errors is preferred, so here is a list of good issues that can be solved:

Start page

http://127.0.0.1:8080/

  • Best practices
    • Browser errors were logged to the console: figure out what produces these errors
  • Performance
    • Serve images in next-gen formats: Add a step to the page build that converts the images to WebP
    • Avoid enormous network payloads: reduce the amount of JavaScript served to the client by lazy loading features

Regular Route: UK→France

http://127.0.0.1:8080/routes/uk-to-france

  • Best practices
    • Browser errors were logged to the console: figure out what produces these errors
  • Accessibility Accessibility #721
    • <frame> or <iframe> elements do not have a title
    • Heading elements are not in a sequentially-descending order
  • Performance
    • Serve images in next-gen formats: Add a step to the page build that converts the images to WebP

Latest Lighthouse result

Latest build of the lighthouse-ci branch

Lighthouse result

Lighthouse result

Page Performance Accessibility Best Practices SEO
/index.html 57 100 92 100
/about-us/index.html 56 100 100 100
/code-of-conduct/index.html 99 100 100 100
/donate/index.html 93 100 100 100
/regions/index.html 95 100 100 96
/regions/eastern-europe/index.html 96 100 100 100
/regions/france/index.html 93 100 100 100
/regions/greece/index.html 91 100 100 100
/regions/lebanon/index.html 87 100 100 100
/regions/the-balkans/index.html 97 100 100 100
/routes/uk-to-france/index.html 68 100 100 98
/routes/uk-to-lebanon/index.html 73 100 100 98
/team/index.html 98 100 100 100
/whistleblowing-policy/index.html 98 100 100 100

Goal

The goals is that these three categories have a 💯 score:

  1. Accessibility
  2. Best Practices
  3. SEO

Performance depends on many factors. So fix the low hanging fruits, then try to achieve what makes sense.

@coderbyheart coderbyheart added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Sep 17, 2022
@raae
Copy link
Contributor

raae commented Sep 18, 2022

Serve images in next-gen formats: Add a step to the page build that converts the images to WebP

When using GatsbyImage this should happen when default format setting is used.

@raae
Copy link
Contributor

raae commented Sep 18, 2022

Avoid enormous network payloads: reduce the amount of JavaScript served to the client by lazy loading features

I have seen this happen on other site if one asks for more data in the page query than is really needed on the page. All page data is also loaded as a page-data.json for when the sites rehydrates into a Single Page Application (SPA).

@preeformance
Copy link
Contributor

Hi @coderbyheart, I have experience and interest in resolving lighthouse errors, especially for accessibility. I noticed on the contributing section, it talked about joining the slack. Is that a pre-requisite for jumping into issues or am I free to go for it now?

@coderbyheart
Copy link
Member Author

Hei @preeformance, not it's not a requirement, we do some org-wide discussions there, but the more detailed tech stuff happens here on GitHub. So feel free to jump in! Thanks for giving it a shot!

@jtfairbank
Copy link
Contributor

jtfairbank commented Oct 2, 2022

Hey @preeformance I assigned this issue to you! Only contribution requirement we have is to add your name to the CONTRIBUTORS.md file when submitting your first PR! That takes care of the code of conduct / copyright stuff. 😄

Thanks for helping out!

@coderbyheart
Copy link
Member Author

Hey @preeformance this is a BIG issue, so don't think you need to fix ALL the issues! Any improvement here is welcome!

@preeformance
Copy link
Contributor

Thanks for that req heads up, @jtfairbank! You commented just in time.

@coderbyheart I appreciate that! I plan to submit PRs page by page for issue areas so that if anything gets wild/broken, it can get tracked. I also have friends who may be interested in helping out with this and I plan to bring them on board.

@coderbyheart
Copy link
Member Author

I solved the image issue on the #699 branch, just using GatsbyImage does not cut it one needs to explicitly request gatsbyImageData in page GraphQL requests, or when rendering Markdown in non-page elements, replace the default URLs. It's doable but takes a bit of effort. The changes I did on the branch (use of GatsbyImage, and using responsive URLs for Markdown) result in a very good score.

Screenshot_20221002_235144

/cc @raae

@coderbyheart
Copy link
Member Author

@preeformance I've decided to unassign you, so others don't get the impression that this is fully covered. /Cc @jtfairbank

@raae
Copy link
Contributor

raae commented Oct 3, 2022

Images in markdown can be replace with the Gatsby Image component using gatsby-remark-images. This requires there to MarkdownRemark nodes with the markdown content.

@coderbyheart
Copy link
Member Author

I tried that but it didn't work for the custom nodes we generate for Forestry content.

@raae
Copy link
Contributor

raae commented Oct 3, 2022

Setting all these values as part of onCreateNode kinda circumvents all the resolver goodies you'll get with the gatsby-transformer-markdown plugin. Such as the html resolver you'll find if you inspect a MarkdownRemark node in the graphiQL explorer, and the way relatives paths in the frontmatter are transformed into links to other File nodes by default etc.

A way to have custom nodes, but also get the all good stuff that happens in these resolvers could be to use this plugin Gatsby plugin: Parent Resolvers defining a schema for the custom nodes instead of setting the values directly on the nodes when creating.

I would be happy to discuss this more on a call or something, as I find it a little hard to explain it all in a short reply like this.

@coderbyheart
Copy link
Member Author

Setting all these values as part of onCreateNode kinda circumvents all the resolver goodies you'll get with the gatsby-transformer-markdown plugin. Such as the html resolver you'll find if you inspect a MarkdownRemark node in the graphiQL explorer, and the way relatives paths in the frontmatter are transformed into links to other File nodes by default etc.

Just did a quick search ... we have 0 inline images in our content right now (regex search for !\[[^\]]+\]\((?<url>[^)]+)\)). So letting MarkdownRemark do the magic won't work. Because we are using Forestry, all images are provided through a field of type file. Those need to be processed manually, I guess. Or can this be done using schema modifications?

@preeformance preeformance mentioned this issue Oct 6, 2022
coderbyheart added a commit that referenced this issue Oct 8, 2022
See #661

Co-authored-by: Markus Tacker <m@coderbyheart.com>
dorian-edwards pushed a commit to dorian-edwards/distributeaid.org that referenced this issue Oct 9, 2022
See distributeaid#661

Co-authored-by: Markus Tacker <m@coderbyheart.com>
@jtfairbank
Copy link
Contributor

Hey @coderbyheart I think I can use the schema modifications like you suggested to load all of the images into the Gatsby API as "GatsbyImage"s. Will try to take a look at this during the week.

@preeformance how's your work on the lighthouse issues going? Don't want to get in your way!

@preeformance
Copy link
Contributor

@jtfairbank I have my eye on the SEO ones next but haven't written anything yet. If you want to go for it, feel free!

@preeformance
Copy link
Contributor

Hi everyone,
I went back to this tonight to check on where things are and I ended up writing a substantial amount. If you're pressed for time, my two main questions are:

  1. If a design change needs to be made, what does that process look like? (see A for details)
  2. Would it be of interest to use a different tool for accessibility to ensure compliance? (see E for details)

If you have a bit more time, here's the full extent of what I noticed:
A. For /regions/index.html, when I ran the lighthouse test for desktop, there was an SEO score of 100 and mobile was 98. The list of places before the "view region" buttons is what's bringing down the score because the links aren't big enough. I think if each place was a button instead of a link, it might pass but it would change the look. I'm happy to give that a shot but felt like that needed a green light first.

B. I ran lighthouse for mobile and desktop on routes/uk-to-france/index.html and it scored 100 on SEO for both.

C. I ran lighthouse for mobile and desktop on routes/uk-to-lebanon/index.html and it also scored 100 on SEO for both.

D. So overall, it seems the scores are higher and I'm not sure why. It doesn't seem like there's been anything on this thread for the past month or so. Maybe PRs from other issues magically fixed some/all of the SEO issues?

E. Recently another dev mentioned to me that Axe is a free accessibility tool similar to lighthouse but with better tests. The dev said that lighthouse only covers accessibility basics while Axe makes sure the site is compliant. I used it on the main landing page and there were 10 "moderate" issues that popped up. If making these changes is of interest, I can either create a separate issue or add another chart here for the Axe results on each page.

@jtfairbank
Copy link
Contributor

@preeformance Thanks so much for staying on this one! And sorry for the delay getting back to you I've had the flu this week.

  1. If the feature isn't public and there's a compelling technical reason to make the change then I think you can call that shot on your own then just point it out in the PR so that a reviewer can take a second look. Right now we care more about functionality and a generally alright aesthetic then pixel perfection. We don't have designs for most pages so they've been someone's best guess.

So it sounds like for /regions you can make those into buttons. Or personally I think the subregion links make things too crowded anyways and you might want to just take em out all together.

  1. Axe sounds awesome, and I think could work well alongside Google Lighthouse for additional A11Y checks. We would want to include Axe in our CI/CD runs, similar to how [ci] enable Lighthouse CI #659 includes Lighthouse in those runs.

My recommendation would be that we focus on resolving the last of the Lighthouse issues and merge #659 in, then basically repeat the process with Axe in a new PR / issue. Can you make an issue to remind us that Axe is a next step?

@coderbyheart
Copy link
Member Author

Closing because I will no longer continue this effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants