style: minor layout spacing tweak for announcement banner #4507
Conversation
atharva038
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdjusted AnnouncementHero Tailwind margin classes in three layout components: changed Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4507 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4507--asyncapi-website.netlify.app/ |
atharva038
changed the title
![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
🧹 Nitpick comments (1)
components/layout/GenericLayout.tsx (1)
43-43: Optional: Remove redundanttext-centerclass.The
text-centerclass in this className prop appears redundant sinceAnnouncementHeroalready appliesclassName='text-center'to its Container element (based on the relevant code snippets).- <AnnouncementHero className={` m-6 text-center ${hideBanner && 'hidden'}`} small={true} /> + <AnnouncementHero className={` m-6 ${hideBanner && 'hidden'}`} small={true} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/layout/GenericLayout.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/layout/GenericLayout.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
components/layout/GenericLayout.tsx
Outdated
| <Container wide={wide}> | ||
| <div data-testid='GenericLayout-banner'> | ||
| <AnnouncementHero className={`m-4 text-center ${hideBanner && 'hidden'}`} small={true} /> | ||
| <AnnouncementHero className={` m-6 text-center ${hideBanner && 'hidden'}`} small={true} /> |
There was a problem hiding this comment.
🧩 Analysis chain
Spacing adjustment looks reasonable. Please verify across pages and screen sizes.
The margin increase from m-4 to m-6 should address the alignment inconsistency. However, since the reported issue (#4495) specifically concerns vertical banner positioning, consider using my-6 (vertical margin only) instead of m-6 (all sides) for a more targeted fix.
Please confirm:
- The banner now aligns consistently between the homepage and other pages
- The spacing appears correct on mobile, tablet, and desktop viewports
- No unintended horizontal spacing issues were introduced
🏁 Script executed:
#!/bin/bash
# Description: Check if homepage uses different margin values for AnnouncementHero
# to understand why alignment differs between pages.
# Search for other usages of AnnouncementHero to compare margin settings
rg -n -C3 'AnnouncementHero' --type=tsx --type=ts -g '!node_modules' -g '!.next'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for AnnouncementHero usages across the codebase
# Using fd to find tsx/ts files and grep to search for the component
echo "=== Searching for AnnouncementHero usages ==="
rg -n -C3 'AnnouncementHero' -g '!node_modules' -g '!.next'
echo ""
echo "=== Viewing GenericLayout.tsx around line 43 ==="
fd 'GenericLayout.tsx' --exec cat {}Length of output: 10978
Incomplete fix: GenericLayout changed to m-6, but GenericPostLayout and GenericWideLayout still use m-4—inconsistency persists.
The margin change in GenericLayout.tsx is inconsistent with similar layout components that still use m-4 (GenericPostLayout.tsx line 35, GenericWideLayout.tsx line 38). If the alignment issue from #4495 affects multiple pages, this change alone may not fully resolve it.
Additionally, consider my-6 (vertical margin) instead of m-6 (all sides). Hero.tsx (line 32) and pages/404.tsx (line 17) use my-4, suggesting a pattern of vertical-only margins that may be more appropriate if the issue is strictly vertical alignment.
🤖 Prompt for AI Agents
In components/layout/GenericLayout.tsx around line 43, the AnnouncementHero
margin was changed to m-6 but related layouts still use m-4 causing
inconsistency; update GenericLayout to use the same margin pattern as
GenericPostLayout.tsx (line 35) and GenericWideLayout.tsx (line 38) — either
standardize all three to m-6 or m-4 as appropriate, and if the alignment issue
is vertical-only, change m-6 to my-6 and update the other two layout files to
the matching my- value so all three components use the same vertical margin
convention.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/layout/GenericLayout.tsx (1)
43-43: Consistent fix for vertical banner alignment.The change from
m-4tomy-4correctly targets vertical margins only, which is appropriate for fixing the vertical alignment issue described in #4495. This now matches the changes in GenericWideLayout and GenericPostLayout, addressing the inconsistency noted in previous reviews.Please verify that the banner now aligns consistently between the homepage and other pages across different screen sizes.
Minor: Leading space in className string.
There's an extra space at the beginning of the className string (
my-4 text-center). While this doesn't affect functionality, consider removing it for cleaner code.- <AnnouncementHero className={` my-4 text-center ${hideBanner && 'hidden'}`} small={true} /> + <AnnouncementHero className={`my-4 text-center ${hideBanner && 'hidden'}`} small={true} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/layout/GenericLayout.tsx(1 hunks)components/layout/GenericPostLayout.tsx(1 hunks)components/layout/GenericWideLayout.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
components/layout/GenericWideLayout.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
components/layout/GenericLayout.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
components/layout/GenericPostLayout.tsx (1)
components/campaigns/AnnouncementHero.tsx (1)
AnnouncementHero(21-112)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (2)
components/layout/GenericWideLayout.tsx (1)
38-38: LGTM! Consistent vertical margin adjustment.The change from
m-4tomy-4is appropriate and aligns with the fixes in the other layout components.components/layout/GenericPostLayout.tsx (1)
35-35: LGTM! Consistent vertical margin adjustment.The change from
m-4tomy-4is appropriate and aligns with the fixes in the other layout components.
2 participants
Description
AnnouncementHero.tsx.Related issue(s)
Resolves #4495
Summary by CodeRabbit