fix: center and properly size event banners regardless of aspect ratio (#746) - #752
fix: center and properly size event banners regardless of aspect ratio (#746)#752Akashstztt wants to merge 5 commits into
Conversation
Project-HAMi#746) Signed-off-by: Akash Kumar <akashstzt@gmail.com>
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Akashstztt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe event banner image is wrapped in ChangesEvent banner layout
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adjusts event banner sizing and centering so square and landscape banners display correctly; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/EventLanding.module.css`:
- Around line 18-23: Update the .banner styling so max-height: 480px applies
only to square and portrait banners, while landscape banners can scale to the
available wrapper width without the global height cap. Preserve intrinsic
aspect-ratio behavior and the existing max-width constraint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 295087f8-3f4b-4f04-8648-36dce6bb989a
📒 Files selected for processing (2)
src/components/EventLanding.jssrc/components/EventLanding.module.css
…ead of conflicting width/height auto Signed-off-by: Akash Kumar <akashstzt@gmail.com>
|
@mesutoezdil @wawa0210 @rootsongjc — this should now be ready for review. I fixed the CodeRabbit comment about the height cap on landscape banners by using a fixed-size wrapper approach. Let me know if you have any concerns! |
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
fix works for the square banner and ci is green. note: #751 touches the same two files in the same hero, coordinate merge order.
Addresses review feedback from @mesutoezdil - the centered 960px banner box looked misaligned above the left-aligned title/description on wide screens. Signed-off-by: Akash Kumar <akashstzt@gmail.com>
…eight Addresses review feedback from @mesutoezdil - height: 480px was letterboxing non-2:1 images and staying rigidly tall on mobile. Switched to max-height + height: auto so the box sizes naturally. Signed-off-by: Akash Kumar <akashstzt@gmail.com>
The max-height: 480px on .bannerWrapper combined with overflow: hidden was clipping the bottom of square/tall banner images even after switching to height: auto. Removed the cap so the wrapper sizes fully to the image's natural aspect ratio. Signed-off-by: Akash Kumar <akashstzt@gmail.com>
|
Noted on the #751 overlap — happy to check its diff and coordinate merge order once it's ready, or rebase on top of it if it merges first. Let me know what you'd prefer. |
|
Closing this for now: #834 addresses the same EventLanding banner sizing issue. If I misread the intent of this change, please say so here and I will reopen it. |
2 participants
Related to #746
What type of PR is this?
/kind bug
What this PR does / why we need it:
The event banner on
/landing/kcd-vietnamwas being cropped because.bannerusedobject-fit: cover, which is designed for wide/landscape images. This event's banner is square, so key visual content (title, speaker names) was getting cut off.Fixed by wrapping the banner image in a flex-centered container and using
max-width/max-heightwithobject-fit: containandwidth/height: auto, so the image sizes itself based on its actual aspect ratio instead of being forced to stretch full-width. This displays square banners fully and centered, while landscape banners (e.g./landing/kubecon-japan) still fill the width naturally.Which issue(s) this PR fixes:
Fixes #746
Checklist:
npm run lintandnpm run format:checkpass — note: both report pre-existing repo-wide issues unrelated to this PR (a Windows markdownlint CLI quirk, and 482 files with existing Prettier formatting drift). Neither of the two files changed in this PR is affected.npm run buildsucceeds for bothenandzh— not verified locally due to a Windows-specificNODE_OPTIONSshell syntax issue in this environment; CI will validate this on Linux.git commit -s)Testing:
/landing/kcd-vietnambanner (square image) now displays fully, with no cropping or letterboxing, sized to its natural aspect ratio./landing/kubecon-japanbanner (landscape image) still displays correctly at full width, with no regression.EventLanding.module.cssis only imported byEventLanding.js, which is only used by these two event pages — no other pages affected by this change.Summary by CodeRabbit