Fix images aspect ratio in catalog #2221
Conversation
Signed-off-by: Nitish C <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @Nitish-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR, authored by Nitish-bot, aims to address an issue with the display of images in the catalog section, specifically within the compatibility filter area. The core problem being fixed is the inconsistent aspect ratio and lack of a consistent bounding box for these images, which can lead to visual inconsistencies and potentially distorted images, as highlighted by the before/after screenshots provided in the description. The changes involve a minor update to the CSS rules targeting these specific images to ensure they maintain their aspect ratio and fit neatly within their designated space.
Highlights
- Image Display Fix: The primary goal of this PR is to fix how images are displayed in the catalog's compatibility filter section. This involves ensuring images maintain their correct aspect ratio and fit within a consistent bounding box.
- CSS Updates: The fix is implemented through a minor CSS modification. The
max-widthof the images is adjusted, and theobject-fitproperty is added to control how the image content is resized to fit its container.
Changelog
- _includes/patterns-filter.html
- Updated the CSS rule
#compatibility-div img. - Changed
max-widthfrom50pxto1.5rem(Line 87). - Added
object-fit: contain;to ensure images maintain aspect ratio and fit within their container (Line 89).
- Updated the CSS rule
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
✅ Deploy Preview for mesheryio-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review
This pull request aims to fix the aspect ratio and add a consistent bounding box for images within the #compatibility-div by adjusting the max-width and adding object-fit: contain. This is a good approach to ensure images are displayed correctly and consistently.
Summary of Findings
- Image size/visibility: The chosen
max-widthvalue (1.5rem) appears to make some images, like the AMD logo, almost invisible, which contradicts the goal of fixing their display. This issue has been raised as ahighseverity comment.
Merge Readiness
This pull request introduces a change intended to fix image display, but the current implementation results in some images being too small, as noted in the PR description and highlighted in a high severity review comment. This issue should be addressed before merging. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.
_includes/patterns-filter.html
Outdated
| @@ -84,8 +84,9 @@ | |||
| } | |||
|
|
|||
| #compatibility-div img { | |||
| max-width: 50px; | |||
| max-width: 1.5rem; | |||
There was a problem hiding this comment.
The max-width has been changed from 50px to 1.5rem. Based on the PR description's note that the AMD logo is "almost invisible" after this change, it seems the 1.5rem value might be too small. Could you reconsider this value to ensure the logos are clearly visible while still maintaining the desired bounding box and aspect ratio?
|
@Nitish-bot Add it as an agenda item to the meeting minutes, if you would :) |
|
@vishalvivekm this is a fairly small change, couldn't get to discuss it on the meet due to time constraints. Can I get a review here? |
|
@Nitish-bot as Gemini noted, isn't 1.5rem too small ? |
|
Yes, that's correct. In my review comment on the specific line where The goal is to fix the aspect ratio and provide a consistent bounding box, but we also need to ensure the logos remain clearly visible and recognizable. Perhaps exploring a slightly larger value or a different approach that balances consistency with logo legibility would be beneficial. The author's note about considering logos without text is also a good point for improving visibility. |
|
@vishalvivekm, if you'll look at the before and after you'll see that no icon that was previously rendered correctly changes width. This is because the icons with aspect ratio 1:1 reach the cap for height before they do for width. This specifically fixes all icons that are wider than they are tall. Also if you'll inspect you'll find their width to always be below 1.5rem even before I made the change. as for the AMD icon being too small, we should change it to the logo with just the icon and no text. |
Signed-off-by: Nitish C <[email protected]>
…atalog-img Signed-off-by: Nitish C <[email protected]>
Signed-off-by: Nitish C <[email protected]>
|
I have changed the icons for AMD in both dark mode and light mode, this small PR was overdue since long. Also we should consider formatting the |
|
Use pixels for SVG image sizing, and rem for text and other elements. |
Signed-off-by: Nitish C <[email protected]>
|
@hortison so I don't think I have the permission to make changes to the spreadsheet, could you or another maintainer take care of that for me please. I have changed the rem value to px value. |
Description
Minor css update to fix aspect ratio and add consistent bounding box
This PR fixes #
Notes for Reviewers
Signed commits