docs: fix announcement banners clshift issue#2671
Conversation
📝 WalkthroughWalkthroughThe announcement banner now retains an outer slot while dismissal state initializes, conditionally renders banner content, and collapses the slot after dismissal. CSS adds variable-driven slot sizing and completes the dark-mode styling block. ChangesAnnouncement banner behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/AnnouncementBanner/AnnouncementBanner.module.css (1)
1-7: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd
overflow: hiddento prevent content overflow.The slot has a fixed height (
4remdefault) but nooverflow: hidden. Multi-line announcement text could exceed the reserved height and overflow into page content below. Addingoverflow: hiddenis a safe defensive measure.♻️ Proposed addition
.announcementSlot { height: var(--ory-announcement-height, 4rem); + overflow: hidden; }🤖 Prompt for 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. In `@src/components/AnnouncementBanner/AnnouncementBanner.module.css` around lines 1 - 7, Add overflow: hidden to the .announcementSlot rule so multi-line announcement content remains clipped within its reserved height, while preserving the existing dismissed-state height behavior.
🤖 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/AnnouncementBanner/AnnouncementBanner.tsx`:
- Line 71: Prevent the announcement banner content from flashing before
dismissal state is loaded. In AnnouncementBanner, update the render condition
around the banner content to require ready and not dismissed, while preserving
the reserved slot layout during initialization; keep the existing ready state
and dismissal effect behavior intact.
---
Nitpick comments:
In `@src/components/AnnouncementBanner/AnnouncementBanner.module.css`:
- Around line 1-7: Add overflow: hidden to the .announcementSlot rule so
multi-line announcement content remains clipped within its reserved height,
while preserving the existing dismissed-state height behavior.
🪄 Autofix (Beta)
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: 4f89563f-bd65-40a3-8482-c9b425052976
📒 Files selected for processing (2)
src/components/AnnouncementBanner/AnnouncementBanner.module.csssrc/components/AnnouncementBanner/AnnouncementBanner.tsx
| data-dismissed={isDismissed ? "true" : "false"} | ||
| > | ||
| <div className="mx-auto flex max-w-screen-xl items-start gap-3 px-4 py-1"> | ||
| {!isDismissed && ( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Flash of banner content for previously-dismissed users.
isDismissed = ready && dismissed is always false on the first render because ready starts false. This means the full banner content renders for one frame before useEffect runs — even for users who previously dismissed it. After the effect, isDismissed becomes true, the content disappears, and the slot collapses from 4rem to 0. This is both a visible content flash and a layout shift for returning users, partially undermining the CLS goal.
Gate content rendering on ready so the slot reserves space without showing content until dismissal state is known:
🛡️ Proposed fix
- {!isDismissed && (
+ {ready && !isDismissed && (This also keeps the ready state meaningful — without it, ready is redundant since dismissed already starts false.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {!isDismissed && ( | |
| {ready && !isDismissed && ( |
🤖 Prompt for 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.
In `@src/components/AnnouncementBanner/AnnouncementBanner.tsx` at line 71, Prevent
the announcement banner content from flashing before dismissal state is loaded.
In AnnouncementBanner, update the render condition around the banner content to
require ready and not dismissed, while preserving the reserved slot layout
during initialization; keep the existing ready state and dismissal effect
behavior intact.
Fixes a Cumulative Layout Shift issue with the announcement banner. This was potentially causing a Google "Core Web Vitals" SEO ranking issue.
Page now renders with enough space dedicated to banner so there is no shifting when the banner renders.
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.com) from the maintainers to push the changes.
Summary by CodeRabbit