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

#Issue 71 #97

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

#Issue 71 #97

wants to merge 4 commits into from

Conversation

margaretmwaura
Copy link
Collaborator

@margaretmwaura margaretmwaura commented Apr 11, 2023

What does this PR DO

The PR is to resolve issue 71. Images are now imported to the index file via getStaticProps.

Help and Collaboration

Since now images are defined by their path and they are not statically imported , a width and a height is required. At the moment a width and a height has been provided.

  • Is there a better way to import the images via getStaticProps or other than getStaticProps?
  • What will be the desired height and width if the current way of importing images is to be adopted?

@vercel
Copy link

vercel bot commented Apr 11, 2023

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

Name Status Preview Comments Updated (UTC)
website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2023 7:36pm

Copy link
Collaborator

@dastrong dastrong left a comment

Choose a reason for hiding this comment

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

@margaretmwaura now we're over-optimizing, by serving a small image and stretching it out to make it bigger. Please update the Image component in ImageMasonry Next.js Image docs to fix this

@margaretmwaura
Copy link
Collaborator Author

@margaretmwaura now we're over-optimizing, by serving a small image and stretching it out to make it bigger. Please update the Image component in ImageMasonry Next.js Image docs to fix this

Okay @dastrong , let me go through the docs

@CollActionOfficial
Copy link
Contributor

Hi Margaret, great to see the progress! Just one thing: could you link your branch to the respective issue (see Developer Essentials on Confluence)? :)

@CollActionOfficial
Copy link
Contributor

Also: sorry I missed your comment at the start @margaretmwaura . Could you tag @tomsiebring next time?

Regarding your comment

Is there a better way to import the images via getStaticProps or other than getStaticProps?
What will be the desired height and width if the current way of importing images is to be adopted?

I'm not sure what the status on this is, but if it helps: we could crop all images to make them square for now. I think what would be ideal is to have images with at most a ratio of 4/3, so 1.333 between width/height or height/width and crop the rest (centering the image, so cropping left/right or top/bottom). However, I can imagine this might be more work, so we could stick to square for now if that helps :) @dastrong what do you think on this?

@CollActionOfficial
Copy link
Contributor

@margaretmwaura how's it going? :)

1 similar comment
@CollActionOfficial
Copy link
Contributor

@margaretmwaura how's it going? :)

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