Lighthouse preset fix #43
Conversation
|
This pull request introduces 1 alert when merging 8bbcd77 into 19373b3 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
==========================================
+ Coverage 95.83% 97.95% +2.12%
==========================================
Files 9 9
Lines 48 49 +1
Branches 4 4
==========================================
+ Hits 46 48 +2
+ Misses 2 1 -1
Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging ebdf279 into b5a954c - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 766ed41 into b5a954c - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 47bb243 into b5a954c - view on LGTM.com new alerts:
|
This reverts commit 47bb243.
|
This pull request introduces 1 alert when merging c0fba91 into b5a954c - view on LGTM.com new alerts:
|
src/pages/landing/index.tsx
Outdated
| import TwitterSvg from "../../assets/icons/twitter.svg?sprite" | ||
| import EmailSvg from "../../assets/icons/email.svg?sprite" | ||
| // @ts-ignore | ||
| import IPhone from "../../../public/iphone_crop.png?resize&sizes[]=300&sizes[]=500&sizes[]=850" |
There was a problem hiding this comment.
Not sure how to make typescript recognise this type - any suggestions?
There was a problem hiding this comment.
Seems like a common issue: cyrilwanner/next-optimized-images#103
You think maybe in the type declaration if we did *.png* would that work?
|
Ready for review: Still one remaining lighthouse failure ("unused-js" - https://web.dev/unused-javascript/), not sure how to fix (checked lighthouse coverage and its not clear which lines its classing as unused), just upped the threshold to 4 for now. Lighthouse flagging a warning for not using next-gen formats - can't fix this with next-optimized-images yet, seems to be a bug that someone else has also seen (cyrilwanner/next-optimized-images#207) |
timothy-ch-cheung
requested review from
pavsidhu and
adamjhc
and removed request for
pavsidhu
package.json
Outdated
| @@ -21,18 +21,22 @@ | |||
| ] | |||
| }, | |||
| "dependencies": { | |||
| "@types/imagemin-svgo": "7.0.0", | |||
| "@types/svg-sprite-loader": "3.9.2", | |||
There was a problem hiding this comment.
Should add these to dev deps instead, same in the package-lock.json
There was a problem hiding this comment.
These dependencies weren't needed (I added them to try to fix the typing of next-optimized-images but forgot to remove them)
src/pages/_document.tsx
Outdated
| @@ -38,6 +38,12 @@ export default class MyDocument extends Document { | |||
| href="/icons/safari-pinned-tab.svg" | |||
| color="#caeaf8" | |||
| /> | |||
| <link rel="manifest" href="/manifest.json" /> | |||
| <meta name="theme-color" content="#fefefe" /> | |||
There was a problem hiding this comment.
Not 100% sure we need this as it's already specified in public/manifest.json should probably double check this
There was a problem hiding this comment.
We do need the tag (see: https://github.com/feelingapp/feeling-app/runs/1168465935?check_suite_focus=true) - I've changed it to match the one in manifest
src/pages/landing/index.tsx
Outdated
| import TwitterSvg from "../../assets/icons/twitter.svg?sprite" | ||
| import EmailSvg from "../../assets/icons/email.svg?sprite" | ||
| // @ts-ignore | ||
| import IPhone from "../../../public/iphone_crop.png?resize&sizes[]=300&sizes[]=500&sizes[]=850" |
There was a problem hiding this comment.
Seems like a common issue: cyrilwanner/next-optimized-images#103
You think maybe in the type declaration if we did *.png* would that work?
Switch to use the recommended lighthouse config, fix findings to get a passing report