feat: add dismissable button to promotional(confrence) banner #4534 #4536
Conversation
amandollar
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. |
WalkthroughAdded a dismissible announcement banner to AnnouncementHero: visibility tracked with Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Component as AnnouncementHero
participant Storage as localStorage
User->>Component: mount
Component->>Storage: read `isVisible` (client-only)
Storage-->>Component: visibility value (or undefined)
Component->>Component: set isVisible (default true), set isLoading = false
alt isLoading = false AND isVisible = true AND visibleBanners exist
Component->>User: render active banner
else
Component-->>User: no banner shown
end
User->>Component: click Close (Cross)
Component->>Component: set isVisible = false
Component->>Storage: persist `isVisible = false`
Component->>User: re-render (banner hidden)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4536--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/campaigns/AnnouncementHero.tsx (2)
103-124: Variable shadowing detected: localisVisibleshadows state variable.Line 105 declares a local
isVisibleconstant that shadows the component's state variable of the same name. This causes linting errors and makes the code confusing.Rename the local variable to avoid shadowing:
{visibleBanners.map((banner, index) => { // Only render the active banner - const isVisible = index === activeIndex; + const isActive = index === activeIndex; - if (!isVisible) return null; + if (!isActive) return null; return ( <Banner key={index} title={banner.title} dateLocation={banner.dateLocation} cfaText={banner.cfaText} eventName={banner.eventName} cfpDeadline={banner.cfpDeadline} link={banner.link} city={banner.city} - activeBanner={isVisible} + activeBanner={isActive} className={className} small={small} /> ); })}
1-150: Address pipeline linting and formatting failures.The GitHub Actions pipeline reports multiple ESLint and Prettier violations including
no-shadow(addressed in separate comment),prettier/prettier,comma-dangle,jsx-quotes, and others.Run the following to auto-fix most formatting issues:
npm run format # or npm run lint:fixThen commit the formatting changes.
🧹 Nitpick comments (1)
components/campaigns/AnnouncementHero.tsx (1)
46-49: Consider removing unnecessary dependency from useMemo.The
bannersimport is a static constant, so including it in the dependency array is unnecessary. The filter will only need to recompute based on time changes (forshouldShowBanner), which aren't tracked here anyway.You can simplify to:
const visibleBanners = useMemo( () => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), - [banners], + [], );Note: This won't trigger on time changes, so if you need dynamic updates based on deadline expiry, consider adding a timer or removing memoization entirely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/campaigns/AnnouncementHero.tsx(4 hunks)next-env.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Applied to files:
next-env.d.ts
📚 Learning: 2025-04-20T16:05:16.482Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Applied to files:
next-env.d.ts
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/campaigns/AnnouncementHero.tsx
🧬 Code graph analysis (1)
components/campaigns/AnnouncementHero.tsx (2)
components/campaigns/banners.ts (2)
banners(18-46)shouldShowBanner(6-16)components/icons/Cross.tsx (1)
Cross(8-22)
🪛 GitHub Actions: PR testing - if Node project
components/campaigns/AnnouncementHero.tsx
[error] 1-105: Linting/formatting errors detected in AnnouncementHero.tsx: multiple prettier/prettier, comma-dangle, newline-after-var, padding-line-between-statements, jsx-quotes, no-shadow, and related ESLint/Prettier issues.
⏰ 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). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (4)
next-env.d.ts (1)
1-6: LGTM! Standard Next.js auto-generated file.This is the standard Next.js TypeScript declaration file. It's auto-generated and follows Next.js conventions correctly.
components/campaigns/AnnouncementHero.tsx (3)
5-5: LGTM! Cross icon import added for close button.The import is correctly added to support the new close button functionality.
36-44: Good use of isLoading guard in localStorage persistence.The
isLoadingguard correctly prevents writing to localStorage during initial mount, avoiding unnecessary writes.
79-81: Verify that isLoading pattern doesn't cause layout shift.The component returns
nullwhileisLoadingistrue, then renders after mount. This could cause Cumulative Layout Shift (CLS) when the banner suddenly appears, contrary to the PR's claim that it "eliminates layout shift."Consider testing the CLS impact in production. If layout shift is occurring, you might need a placeholder or skeleton during the loading state instead of returning
null.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/campaigns/AnnouncementHero.tsx (1)
103-124: Fix variable shadowing ofisVisible.The local variable
isVisible(line 105) shadows the state variableisVisible(line 28), making the code confusing and triggering an ESLint error.Apply this diff to rename the local variable:
{visibleBanners.map((banner, index) => { // Only render the active banner - const isVisible = index === activeIndex; + const isActive = index === activeIndex; - if (!isVisible) return null; + if (!isActive) return null; return ( <Banner key={index} title={banner.title} dateLocation={banner.dateLocation} cfaText={banner.cfaText} eventName={banner.eventName} cfpDeadline={banner.cfpDeadline} link={banner.link} city={banner.city} - activeBanner={isVisible} + activeBanner={isActive} className={className} small={small} /> ); })}
🧹 Nitpick comments (1)
components/campaigns/AnnouncementHero.tsx (1)
30-30: Consider extracting localStorage key to a constant.The localStorage key
'announcementBannerVisible'is hard-coded in multiple places. Extracting it to a constant improves maintainability.Add at the top of the file:
const BANNER_VISIBILITY_KEY = 'announcementBannerVisible';Then use it:
const stored = localStorage.getItem(BANNER_VISIBILITY_KEY); // ... localStorage.setItem(BANNER_VISIBILITY_KEY, isVisible.toString());Also applies to: 42-42
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/campaigns/AnnouncementHero.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/campaigns/AnnouncementHero.tsx
🧬 Code graph analysis (1)
components/campaigns/AnnouncementHero.tsx (3)
components/campaigns/banners.ts (2)
banners(18-46)shouldShowBanner(6-16)components/icons/ArrowLeft.tsx (1)
ArrowLeft(7-17)components/icons/Cross.tsx (1)
Cross(8-22)
🪛 GitHub Actions: PR testing - if Node project
components/campaigns/AnnouncementHero.tsx
[error] 22-24: Prettier formatting issue: Replace ⏎··className·=·'',⏎··small·=·false,⏎ with ·className·=·'',·small·=·false·
[error] 24-24: Prettier: Unexpected trailing comma. comma-dangle
[error] 30-30: ESLint: Expected blank line after variable declarations. newline-after-var
[error] 31-31: ESLint: Expected blank line before this statement. padding-line-between-statements
[error] 33-33: ESLint: Expected blank line before this statement. padding-line-between-statements
[error] 46-46: Prettier: Replace ⏎····()·=>·banners.filter((banner)·=>·shouldShowBanner(banner.cfpDeadline)),⏎····[banners],⏎·· with ()·=>·banners.filter((banner)·=>·shouldShowBanner(banner.cfpDeadline)),·[banners]
[warning] 48-48: React Hook: useMemo has an unnecessary dependency: 'banners'.
[error] 48-48: Prettier: Unexpected trailing comma. comma-dangle
[error] 53-53: Prettier: Replace ⏎······prevIndex·===·0·?·numberOfVisibleBanners·-·1·:·prevIndex·-·1,⏎···· with ·(prevIndex·===·0·?·numberOfVisibleBanners·-·1·:·prevIndex·-·1)
[error] 54-54: Prettier: Unexpected trailing comma. comma-dangle
[error] 59-59: Prettier: Replace ⏎······prevIndex·===·numberOfVisibleBanners·-·1·?·0·:·prevIndex·+·1,⏎···· with ·(prevIndex·===·numberOfVisibleBanners·-·1·?·0·:·prevIndex·+·1)
[error] 60-60: Prettier: Unexpected trailing comma. comma-dangle
[error] 69-69: Prettier: Replace ()=>setActiveIndex((index)=> (index+1) % numberOfVisibleBanners),⏎······10000,⏎···· with ()=>setActiveIndex((index)=> (index+1) % numberOfVisibleBanners),·10000
[error] 71-71: Prettier: Unexpected trailing comma. comma-dangle
[error] 84-84: Prettier: Replace "section" padding="" className="text-center" with 'section' padding='' className='text-center'
[error] 84-84: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 85-85: Prettier: Replace "relative·flex·flex-row·items-center·justify-center·overflow-x-hidden·md:gap-4" with 'relative·flex·flex-row·items-center·justify-center·overflow-x-hidden·md:gap-4'
[error] 85-85: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 92-92: Prettier: Replace "text-white" with 'text-white'
[error] 92-92: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 95-95: Prettier: Replace "relative·flex·w-4/5·md:w-5/6·flex-col·items-center·justify-center·gap-2" with 'relative·flex·w-4/5·md:w-5/6·flex-col·items-center·justify-center·gap-2'
[error] 95-95: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 96-96: Prettier: Replace "relative·flex·min-h-72·w-full·justify-center·overflow-hidden·lg:h-[17rem]·lg:w-[38rem]" with 'relative·flex·min-h-72·w-full·justify-center·overflow-hidden·lg:h-[17rem]·lg:w-[38rem]'
[error] 96-96: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 98-98: Prettier: Replace "absolute·right-2·top-6·z-20·cursor-pointer·p-2·hover:opacity-70" with 'absolute·right-2·top-6·z-20·cursor-pointer·p-2·hover:opacity-70'
[error] 98-98: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 105-105: ESLint: No shadowing of variable 'isVisible' in outer scope.
[error] 126-126: Prettier: Replace "m-auto·flex·justify-center" with 'm-auto·flex·justify-center'
[error] 126-126: Prettier: Unexpected usage of doublequote. jsx-quotes
[error] 144-144: Prettier: Replace "text-white" with 'text-white'
[error] 144-144: Prettier: Unexpected usage of doublequote. jsx-quotes
🔇 Additional comments (4)
components/campaigns/AnnouncementHero.tsx (4)
5-5: LGTM!The Cross icon import is correctly added and used in the close button implementation.
52-66: Navigation logic is correct.The cycling behavior correctly wraps around at boundaries for both previous and next navigation.
79-81: Conditional rendering logic is correct.The addition of
!isVisiblecheck properly prevents rendering when the banner is dismissed.
22-25: Verify formatting violations in your local environment before proceeding.The suggested commands could not be fully validated in the sandbox environment. However, examining the file directly shows the code appears properly formatted with:
- Single quotes used consistently
- Proper spacing and structure
- No obvious trailing commas or formatting issues
To confirm whether violations actually exist:
- Run
npm run lintin your local environment to identify any real violations- If violations are found, run
npm run lint:fix(the correct script for this project) to auto-fix themNote:
npm run formatdoes not exist in this project's scripts. Usenpm run lint:fixinstead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4536 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 780 780
Branches 144 144
=========================================
Hits 780 780 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/campaigns/AnnouncementHero.tsx (2)
24-57: State management correctly prevents hydration mismatches.The implementation properly avoids SSR/client hydration issues by:
- Rendering
nullduring SSR and initial client render (viaisLoading)- Reading localStorage only after mount in the first effect
- Persisting changes only after the initial read completes
However, the logic is complex with two interdependent effects. Consider simplifying:
- const [isLoading, setIsLoading] = useState(true); - - // Start with a stable server-friendly default. Defer reading localStorage until after mount const [isVisible, setIsVisible] = useState(true); + const [isHydrated, setIsHydrated] = useState(false); - // Read persisted preference from localStorage on mount (client-only) useEffect(() => { try { const stored = localStorage.getItem('announcementBannerVisible'); - - if (stored == null) { - // no persisted preference - } else { - const val = stored === 'true'; - - setIsVisible((prev) => (prev === val ? prev : val)); + if (stored !== null) { + setIsVisible(stored === 'true'); } } catch (e) { // ignore localStorage errors } finally { - setIsLoading(false); + setIsHydrated(true); } }, []); - // Persist visibility changes after initial mount to avoid SSR/client divergence useEffect(() => { - if (!isLoading) { + if (isHydrated) { try { localStorage.setItem('announcementBannerVisible', isVisible.toString()); } catch (e) { // ignore write errors } } - }, [isVisible, isLoading]); + }, [isVisible, isHydrated]);Then update line 82:
- if (isLoading || numberOfVisibleBanners === 0 || !isVisible) { + if (!isHydrated || numberOfVisibleBanners === 0 || !isVisible) {This is clearer:
isHydratedexplicitly communicates "client-side initialization complete" rather than the double-negative!isLoading. The conditional setState optimization on line 39 can also be removed since React batches updates efficiently.
99-106: Consider defensive button styling.The button correctly uses semantic HTML and has proper accessibility attributes. However, it might benefit from explicit style resets to ensure consistent rendering across browsers:
<button type='button' - className='absolute right-2 top-6 z-20 p-2 hover:opacity-70' + className='absolute right-2 top-6 z-20 border-0 bg-transparent p-2 hover:opacity-70' onClick={() => setIsVisible(false)} aria-label='Close announcement' >Adding
border-0 bg-transparentexplicitly removes default button styling, though Tailwind's reset likely already handles this.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/campaigns/AnnouncementHero.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/campaigns/AnnouncementHero.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/campaigns/AnnouncementHero.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/campaigns/AnnouncementHero.tsx
🧬 Code graph analysis (1)
components/campaigns/AnnouncementHero.tsx (2)
components/campaigns/banners.ts (2)
banners(18-46)shouldShowBanner(6-16)components/icons/Cross.tsx (1)
Cross(8-22)
⏰ 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). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (3)
components/campaigns/AnnouncementHero.tsx (3)
59-59: Empty dependency array is correct.The empty dependency array is appropriate since
bannersis a static import. Note that this meansvisibleBannersis computed once on mount, so banners won't disappear mid-session if a deadline passes—only on page refresh. This is acceptable UX for promotional content.
107-128: Banner rendering logic is correct.The early return pattern efficiently renders only the active banner. Note that
activeBanner={isActiveBanner}always passestruesince we returnnullfor inactive banners. This is likely intentional—the Banner component may use this prop for animations or transitions.
82-84: Early return correctly prevents hydration issues and flash.The
isLoadingcheck ensures the component rendersnullduring SSR and initial client render, preventing hydration mismatches while the localStorage preference is being read.
|
@asyncapi-bot @antoniogarrote @MikeRalphson @scharrier Note: The macOS-13 runner is deprecated and causing brownout failures in this PR. |
amandollar
changed the title
2 participants
Problem
Users need a way to dismiss the announcement banner when they don't want to see it anymore.
Solution
Added a cross button in the top-right corner that allows users to close the banner, with state persistence across page refreshes.
Key Changes
Testing Done
Performance Impact
✅ Eliminates layout shift (CLS improvement)
✅ Reduces unnecessary re-renders
✅ Optimizes component mount behavior
Screenshots/Videos
| Before | After |
Files Changed
components/campaigns/AnnouncementHero.tsxReview Focus
Relates to #4534
Summary by CodeRabbit