Fixed sdg image grid firefox

[EdASalazar]

Fixes #4574

What changes did you make and why did you make them ?

• I did some research and found a similar issue on StackOverflow. Chrome gives the images a height but Firefox doesn't.
• In the _about.scss I added lines 47 to 50. I gave the images a height of 140px. This resolved the issue.
• There might be another solution. You could also remove both the "justified-item: stretch" and "align-item: space-between" from the "grid-2-container" class in _about.scss. Both solutions seem to give similar results. Not sure which is more "Right."

Details

Chrome and Firefox were displaying images differently on the About page. In Firefox the images in the container were shifted down and not completely visible.

Before the changes

After the changes the images render correctly and are consistent with how Chrome renders the same page.

[github-actions]

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b EdASalazar-fix-SDG-image-grid-Firefox-4574 gh-pages
git pull https://github.com/EdASalazar/website.git fix-SDG-image-grid-Firefox-4574

[steven-positive-tran]

Avail: Sunday, Monday
ETA: Monday 11PM

[Adastros]

Availability (PST): 11 PM 08MAY2023 - 12 AM 09MAY2023
Review ETA (PST): 12 AM 09MAY2023

[steven-positive-tran]

• Looks good you have done a pull request from your fork to the gh-pages branch in main.
• Works on my machine with both firefox and in edge
• Code seems inline with what the issue needed.
• I don't see any elements in the other pages that use that grid-2-column so no other pages should be affected.

Thanks for taking up this issue for hackforla.

[Adastros]

Hi @EdASalazar, good job on figuring out the problem with the image grid on Firefox!

Below are my thoughts:

• I think rather than setting the image height to 140 px, it would be better to set the height to 100%.
  • Doing so fixes the image grid while keeping the images in the grid responsive at all viewport widths.
  • When the image grid is updated to account for the correct spacing per the Figma, the height and widths of the images on the webpage are pretty close to the specified height and widths on the Figma design (off by about a pixel or two). I think in this case it's okay to deviate from the Figma design a little to avoid having to specify a specific height and width for the images at various breakpoints.
• Since adding a height fixes the image grid, justify-items: stretch and align-content: space-between can be removed since these have no effect on the positioning of the images.
• width: auto can also be removed as well since it doesn't have any visual effects. I think this might be due to max-width: 100%.
• Although the images look big between the viewport widths of ~600 - 959 px, it's better than having small images in the middle of the page due to the set height of 140px. See images below for examples.
  • If we want to avoid the large images, I think it might help to have the images remain to the left of the text similar to the desktop view based on how much empty space there is. This will need to be addressed in a separate issue.

Viewport width ~600 - 959 px - Height: 100%

Viewport width ~600 - 959 px - Height: 140px

[EdASalazar]

@Adastros thank you for the feedback. I was probably a little more gun-shy about deleting the "stretch" and "space-between" than I should've been. LOL

Thanks for the feedback about the image size.

Do I need to do anything further? Should I make those changes and do another pull request?

[Adastros]

Hi @EdASalazar, no problem! Feel free to make changes to the code if you think it'll improve the code and the changes are relevant to the issue you're working on.

As @steven-positive-tran mentioned in the meeting, you can make the changes, commit, and push it to the same branch linked in this pull request. No need to create a new pull request. Your changes will update automatically. You can verify it applied by clicking on the "commits" and/or "files changed" tabs for this pull request.

Sorry for the interruption in the meeting, I was thinking about this today and realized that the height: 100% isn't needed too. Removing the justify-items: stretch and align-content: space-between should be enough since CSS grid automatically adjusts the height and width. It's most likely that justify-items: stretch and align-content: space-between were causing the images to behave oddly. Adding height: 140px likely fixed the issue by forcing the images to be that height.

[EdASalazar]

Made the requested changes and pushed.

[Adastros]

Hi @EdASalazar, sorry for the confusion! I think it's best to remove lines 42 - 48 per my last comment. The grid behaves as expected on Firefox and Chrome without the height: 100%.

max-width: 100% doesn't need to be added to the images. I meant to say that if you inspect the images with the browser developer tool, you can see that max-width: 100% is already applied twice to img elements somewhere else in our CSS code. I'm thinking this is what is helping to contain the images inside the grid cells so width: auto doesn't seem to have an effect.

[EdASalazar]

Hi @EdASalazar, sorry for the confusion! I think it's best to remove lines 42 - 48 per my last comment. The grid behaves as expected on Firefox and Chrome without the height: 100%.

max-width: 100% doesn't need to be added to the images. I meant to say that if you inspect the images with the browser developer tool, you can see that max-width: 100% is already applied twice by some classes somewhere else in our CSS code. I'm thinking this is what is helping to contain the images inside the grid cells so width: auto doesn't seem to have an effect.

Got it. Do you want me to delete the width and update the pull request? If so this is what will have been changed:

Removed "justify-items: stretch" from .grid-2-column.
Removed "align-content: space-between" from .grid-2-column.

Added "height: 100%" to .grid-2-column > img.

[EdASalazar]

I deleted:

grid-2-column > img {
height: 100%;
max-width: 100%;
}

[Adastros]

Hi @EdASalazar, thanks for updating it! The grid functions on Firefox as intended