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

16 utilize vercel image optimizations #24

Merged
merged 22 commits into from
Oct 4, 2023

Conversation

kobebuckley
Copy link
Contributor

Update representing the Image implementation suggestions from Next js for optimization as well as required alt tag additions with relevant descriptions

@kobebuckley kobebuckley linked an issue Sep 19, 2023 that may be closed by this pull request
Copy link
Contributor

@MichaelTamaki MichaelTamaki left a comment

Choose a reason for hiding this comment

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

LGTM! Have suggestions but they are not blockers 💯

src/components/HomePage.js Outdated Show resolved Hide resolved
src/components/Logo.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@Alexandra-Haynes Alexandra-Haynes left a comment

Choose a reason for hiding this comment

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

@kobebuckley , thank you for this PR 🔥 I think transitioning to the Image component has some strong advantages over the standard tag.

I've noticed a couple of areas we could improve with the component usage:

1. Alt Text: I agree with @MichaelTamaki that the alt description for the images are quite detailed, which is great, but it might be a bit lengthy for accessibility users. Consider simplifying it for clarity. Here's a suggestion: alt="Code with Aloha logo featuring an orange lightbulb." or simply alt="Code with Aloha logo" Keeping the alt description short provides efficiency for screen readers that vocalize the alt text and it also quickly provides the context and relevance of the image, without overwhelming the user.

2. Width & Height Properties:

For the next/image component, it's essential to specify the intrinsic dimensions of the image. Without specifying them, we get a server error: Error: Image with src "../logo.png" is missing required "width" property. For these properties we have to look at the natural width and heights of the pics (by looking at the properties), this will assure the natural ratio. The actual rendered dimensions will be controlled by tailwind classes. This means the image won't look stretched or squished.
That being said, for the logo dimensions are 500px by 500px and for the hero_image: 1300px by 1300px. Please add these dimensions as width and height attributes.

I hope this feedback helps in refining the code. Let me know your thoughts!

kobebuckley and others added 2 commits September 19, 2023 21:00
Co-authored-by: Michael Tamaki <michaeltamakidev@gmail.com>
Co-authored-by: Michael Tamaki <michaeltamakidev@gmail.com>
@kobebuckley
Copy link
Contributor Author

kobebuckley commented Sep 20, 2023

LGTM! Have suggestions but they are not blockers 💯

Thank you for providing examples, links, and suggestions for fixing up the code as shown above. I will implement these changes moving forward

@kobebuckley
Copy link
Contributor Author

Thank you Alexandra; I'm still working on the 2nd one, but I'll get it fixed

@Alexandra-Haynes
Copy link
Collaborator

Kobe, thanks for working on this! I was reviewing your pull request and noticed the Image component usage in your code.

In Next.js, the width and height props of the Image component should be written with curly braces instead of quotation marks. See example here: https://nextjs.org/docs/pages/api-reference/components/image#width This syntax is used because the values for these props are expected to be numbers, not strings.

Would you mind updating the width and height to use curly braces like this? width={500} height={500}

This change will ensure that the props are passed as numbers and help to avoid any potential issues related to prop types.

Thanks again for your work, and let me know if you have any questions!

@kobebuckley
Copy link
Contributor Author

Kobe, thanks for working on this! I was reviewing your pull request and noticed the Image component usage in your code.

In Next.js, the width and height props of the Image component should be written with curly braces instead of quotation marks. See example here: https://nextjs.org/docs/pages/api-reference/components/image#width This syntax is used because the values for these props are expected to be numbers, not strings.

Would you mind updating the width and height to use curly braces like this? width={500} height={500}

This change will ensure that the props are passed as numbers and help to avoid any potential issues related to prop types.

Thanks again for your work, and let me know if you have any questions!

Thank you Alexandra! I replaced the string values with the proper prop types for numbers instead to represent Next js syntax

@Alexandra-Haynes Alexandra-Haynes added the hacktoberfest-accepted PR eligible for Hacktoberfest 2023 label Oct 4, 2023
Copy link
Collaborator

@Alexandra-Haynes Alexandra-Haynes left a comment

Choose a reason for hiding this comment

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

Thank you, Kobe! We appreciate your help

@Alexandra-Haynes Alexandra-Haynes merged commit 3000e52 into main Oct 4, 2023
1 check passed
@Alexandra-Haynes Alexandra-Haynes deleted the 16-utilize-vercel-image-optimizations branch October 4, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PR eligible for Hacktoberfest 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utilize Vercel Image optimizations
3 participants