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.

Sign up for GitHub

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

Jump to bottom

Lighthouse best practices & accessibility #131

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Lighthouse best practices & accessibility #131

wants to merge 8 commits into from

Conversation

Tschonti
Copy link
Collaborator

@Tschonti Tschonti commented Dec 13, 2024 •
edited
Loading

Things changed:

  • generate source maps in production: it's an open source app, so why not. The check still fails on the preview, I guess the env is not production, should work when merged
  • fixed a few warnings (highcharts accessibility module included, deprecated meta tag removed)
  • CSP: only allow scripts, styles from certain domains. This is a bit hard, because next adds a bunch of scripts and styles dynamically, so inline scripts and styles are also allowed, which kinda defeats the purpose, but Lighthouse is satisfied. Also the netlify preview features no longer work because netlify's script is not allowed, but we were'nt using that anyway

remaining issues:

  • some warnings because preloaded images or fonts are not used instantly - I think we can ignore these, we have a reason to preload them

@netlify Netlify
Copy link

netlify bot commented Dec 13, 2024 •
edited
Loading

✅ Deploy Preview for wfp-hungermap ready!

Name Link
🔨 Latest commit be07d23
🔍 Latest deploy log https://app.netlify.com/sites/wfp-hungermap/deploys/67681746ff0f3c000836e205
😎 Deploy Preview https://deploy-preview-131--wfp-hungermap.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 configuration.

@Tschonti Tschonti marked this pull request as ready for review December 14, 2024 11:57
@Tschonti Tschonti requested a review from marinovl7 December 14, 2024 11:57
marinovl7
marinovl7 requested changes Dec 15, 2024
View reviewed changes
Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

Just one small change. The Lighthouse score fails for accessibility, idk if its hapenning on prod, but lets have a look tomorrow in the demos and then if it works fine and we defined the next actions we will merge this

src/components/Accordions/AccordionBoxItems.tsx Outdated
Comment on lines 83 to 91
// <div
// role="button"
// tabIndex={0}
// onClick={() => 1}
// onKeyDown={() => 1}
// className="w-[37px] h-[37px] p-[5.5px] hover:bg-default hover:opacity-40 rounded-xl"
// >
// <span className="hover:opacity-100 hover:text-white">{item.infoIcon}</span>
// </div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this please

@Tschonti Tschonti mentioned this pull request Dec 15, 2024
Open
@Tschonti Tschonti requested a review from marinovl7 December 22, 2024 13:33
@Tschonti
Copy link
Collaborator Author

best practices score should be 100 on master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marinovl7 marinovl7 Awaiting requested review from marinovl7

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tschonti @marinovl7