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

Vulcan: Add html image dimensions for lighthouse #1781

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

ianrohde
Copy link
Contributor

@ianrohde ianrohde commented Jun 28, 2023

Issue

We're getting dinged in Lighthouse for not having html image dimensions applied. Let's add them.

Modified

  1. add html image dimensions for lighthouse
  2. add lazyloading to Problem images

CR

http://localhost:3000/Vulcan/Dryer_Not_Spinning, etc

Screenshot 2023-06-28 at 11 00 38 AM Screenshot 2023-06-28 at 11 03 18 AM

I swapped the image border in Problem.tsx to outline so we can apply dimensions directly and not have to worry about the extra 2px a border would add.

Linked to https://github.com/iFixit/ifixit/issues/48516

qa_req 0 (produces no visual changes)

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-commerce ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 28, 2023 6:17pm
react-commerce-prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 28, 2023 6:17pm

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2023

📦 Next.js Bundle Analysis for @ifixit/commerce-frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

- low-hanging prop addition to fix another lighthouse issue
objectFit="cover"
src={imageUrl}
alt=""
outline="1px solid"
outlineColor="gray.300"
Copy link
Contributor

@hcutrone hcutrone Jun 28, 2023

Choose a reason for hiding this comment

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

Why did you change border to outline?

Oops I didn't read the CR notes first

Copy link
Contributor

@hcutrone hcutrone left a comment

Choose a reason for hiding this comment

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

CR 🌯 with a minor question

@deltuh-vee deltuh-vee merged commit d920ace into main Jun 28, 2023
13 checks passed
@deltuh-vee deltuh-vee deleted the Vulcan--add-html-image-dimensions branch June 28, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants