feat: add dismissible close button to conference banner #4537
Conversation
SHASHI9705
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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
WalkthroughThe PR adds client-side banner dismissal functionality to the AnnouncementHero component with localStorage persistence. Banners can be individually closed via a close button and filtered from view. The component maintains carousel behavior while rendering only the active banner. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant LocalStorage
participant UI
User->>Component: Mount component
Component->>LocalStorage: Load bannerClosed & dismissedBanners
LocalStorage-->>Component: Return stored values
Component->>Component: Compute visibleBanners (filter by shouldShowBanner & dismissedBanners)
Component->>UI: Render active banner with close button
User->>UI: Click close button
UI->>Component: Trigger handleClose()
Component->>LocalStorage: Persist current banner as dismissed
LocalStorage-->>Component: Confirm save
Component->>Component: Recompute visibleBanners (exclude dismissed)
alt Banners remain
Component->>Component: Update activeIndex to valid banner
Component->>UI: Re-render with new active banner
else No banners remain
Component->>Component: Set isBannerVisible = false
Component->>UI: Hide entire banner component
end
UI-->>User: Display updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-10-11T10:46:58.882ZApplied to files:
🧬 Code graph analysis (1)components/campaigns/AnnouncementHero.tsx (2)
🪛 GitHub Actions: PR testing - if Node projectcomponents/campaigns/AnnouncementHero.tsx[error] 27-27: Unexpected negated condition. (no-negated-condition) [error] 61-61: Expected blank line after variable declarations. (newline-after-var) [error] 62-62: Expected blank line before this statement. (padding-line-between-statements) [error] 65-65: Replace [error] 77-77: Expected blank line after variable declarations. (newline-after-var) [error] 77-77: Replace [error] 81-81: Expected blank line before this statement. (padding-line-between-statements) [error] 89-89: Replace [error] 89-89: Unexpected usage of doublequote. (jsx-quotes) [error] 89-89: Unexpected usage of doublequote. (jsx-quotes) [error] 89-89: Unexpected usage of doublequote. (jsx-quotes) [error] 90-90: Replace [error] 90-90: Unexpected usage of doublequote. (jsx-quotes) [error] 97-97: Replace [error] 97-97: Unexpected usage of doublequote. (jsx-quotes) [error] 101-101: Replace [error] 101-101: Unexpected usage of doublequote. (jsx-quotes) [error] 102-102: Replace [error] 102-102: Unexpected usage of doublequote. (jsx-quotes) [error] 107-107: Replace [error] 107-107: Unexpected usage of doublequote. (jsx-quotes) [error] 111-111: Replace [error] 111-111: Unexpected usage of doublequote. (jsx-quotes) [error] 112-112: This line has a length of 199. Maximum allowed is 120. (max-len) [error] 112-112: Replace [error] 112-112: Unexpected usage of doublequote. (jsx-quotes) 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-4537--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: 5
🧹 Nitpick comments (1)
components/campaigns/AnnouncementHero.tsx (1)
81-87: Close button implementation looks good with minor suggestions.The close button has proper accessibility (aria-label) and styling. Consider these optional enhancements:
- Add keyboard support for Escape key to close banner
- Consider using an icon component instead of the × character for consistency
- Add focus styles for better keyboard navigation visibility
Example keyboard support:
useEffect(() => { const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape' && isBannerVisible) { handleClose(); } }; document.addEventListener('keydown', handleEscape); return () => document.removeEventListener('keydown', handleEscape); }, [isBannerVisible]);
📜 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: 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 a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
next-env.d.ts
🧬 Code graph analysis (1)
components/campaigns/AnnouncementHero.tsx (1)
components/campaigns/AnnouncementBanner.tsx (1)
Banner(37-69)
🪛 GitHub Actions: PR testing - if Node project
components/campaigns/AnnouncementHero.tsx
[error] 27-27: eslint: Expected blank line after variable declarations. newline-after-var
[error] 28-28: eslint: Expected blank line before this statement. padding-line-between-statements
[warning] 31-31: React Hook useMemo has an unnecessary dependency: 'banners'.
[error] 52-52: eslint: Expected blank line after variable declarations. newline-after-var
[warning] 53-53: eslint: Expected blank line before this statement. padding-line-between-statements
⏰ 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: Lighthouse CI
🔇 Additional comments (1)
components/campaigns/AnnouncementHero.tsx (1)
51-54: Fix required: Missing blank lines causing lint failures.Pipeline is failing due to missing blank lines around the interval cleanup.
Apply this diff:
useEffect(() => { const interval = setInterval(() => setActiveIndex((index) => (index + 1) % numberOfVisibleBanners), 10000); + return () => clearInterval(interval); }, [numberOfVisibleBanners]);Likely an incorrect or invalid review comment.
| const handleClose = () => { | ||
| setIsBannerVisible(false); | ||
| localStorage.setItem('bannerClosed', 'true'); | ||
| }; |
There was a problem hiding this comment.
Consider banner-specific dismissal instead of global dismissal.
The current implementation dismisses ALL banners permanently when the close button is clicked, even if the user only intended to dismiss the currently visible banner. Once closed, the banner(s) remain hidden indefinitely until localStorage is manually cleared, which prevents users from seeing new conference announcements.
Consider implementing one of these approaches:
- Store dismissal state per banner (e.g., using banner ID/title as key)
- Add a time-based reset (e.g., show again after 7 days)
- Store the specific conference that was dismissed
Example implementation for banner-specific dismissal:
const handleClose = () => {
const currentBanner = visibleBanners[activeIndex];
const dismissedBanners = JSON.parse(localStorage.getItem('dismissedBanners') || '[]');
dismissedBanners.push(currentBanner.eventName);
localStorage.setItem('dismissedBanners', JSON.stringify(dismissedBanners));
// Filter out dismissed banners
const remainingBanners = visibleBanners.filter(
b => !dismissedBanners.includes(b.eventName)
);
if (remainingBanners.length === 0) {
setIsBannerVisible(false);
} else {
// Move to next banner or wrap around
setActiveIndex(prev => prev % remainingBanners.length);
}
};Then update the initial load effect to filter dismissed banners from visibleBanners.
🤖 Prompt for AI Agents
In components/campaigns/AnnouncementHero.tsx around lines 46-49, the close
handler currently sets a global 'bannerClosed' flag which hides all banners
permanently; change it to store dismissal per banner (e.g., by eventName or id)
in localStorage (use a JSON array like 'dismissedBanners'), update handleClose
to add the current banner's identifier to that array and then filter
visibleBanners to remove dismissed ones (and adjust activeIndex safely or hide
the banner container if none remain), and modify the component's initial
load/effect to read and filter visibleBanners by that dismissed list (optionally
include a timestamp per entry if you prefer time-based reset).
| /// <reference types="next" /> | ||
| /// <reference types="next/image-types/global" /> | ||
| /// <reference path="./.next/types/routes.d.ts" /> | ||
|
|
||
| // NOTE: This file should not be edited | ||
| // see https://nextjs.org/docs/pages/api-reference/config/typescript for more information. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this auto-generated file should be version controlled.
This file is auto-generated by Next.js and typically should not be manually edited or committed to version control (usually listed in .gitignore). However, some projects do commit it for consistency.
Run this script to check if next-env.d.ts is already in .gitignore:
🏁 Script executed:
#!/bin/bash
# Check if next-env.d.ts should be in .gitignore
grep -n "next-env.d.ts" .gitignore || echo "next-env.d.ts not found in .gitignore"Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check if next-env.d.ts exists and is tracked
echo "=== Checking if next-env.d.ts exists ==="
ls -la next-env.d.ts 2>/dev/null || echo "File does not exist"
echo ""
echo "=== Checking git status of next-env.d.ts ==="
git ls-files --cached next-env.d.ts 2>/dev/null && echo "File is tracked in git" || echo "File is not tracked"
echo ""
echo "=== Checking .gitignore content for auto-generated patterns ==="
cat .gitignore | head -50Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check git diff for changes to next-env.d.ts
echo "=== Git diff for next-env.d.ts ==="
git diff HEAD -- next-env.d.ts 2>/dev/null | head -50 || echo "No diff found"
echo ""
echo "=== Check if file is added (untracked) ==="
git status --short 2>/dev/null | grep -i "next-env" || echo "No untracked next-env files"
echo ""
echo "=== Check if this is a Next.js project ==="
test -f package.json && echo "package.json exists" || echo "No package.json"
test -f next.config.js && echo "next.config.js exists" || echo "No next.config.js"
test -f tsconfig.json && echo "tsconfig.json exists" || echo "No tsconfig.json"
echo ""
echo "=== List TypeScript declaration files ==="
find . -maxdepth 2 -name "*.d.ts" 2>/dev/null | head -10Length of output: 268
Add next-env.d.ts to .gitignore.
This auto-generated file should not be committed to version control. Add next-env.d.ts to .gitignore so it's regenerated locally during builds instead of being versioned.
🤖 Prompt for AI Agents
In next-env.d.ts around lines 1 to 6, the auto-generated Next.js type file is
currently tracked in git; update the repository's .gitignore to include
"next-env.d.ts" so this file is not committed. Edit the .gitignore at the repo
root and add a line with next-env.d.ts, then remove the tracked file from the
index with git rm --cached next-env.d.ts (or equivalent) so it will be
regenerated locally during builds.
|
Hi @SHASHI9705, |
Ok sir, i'll do it asap. |
Done sir. |
3 participants
Description
Added a dismissible "×" close button to the conference banner to improve user control.
Implementation
useStatefor banner visibility.localStorageto persist user’s choice.Screenshot
Before vs After
Summary by CodeRabbit
New Features
Chores