feat: create banner for online conf #3312
Conversation
thulieblack
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnnouncementHero
participant BannerComponent
participant YouTube
participant LinkedIn
User->>AnnouncementHero: Request to view banners
AnnouncementHero->>BannerComponent: Check visible banners
BannerComponent->>YouTube: Check banner availability
BannerComponent->>LinkedIn: Check banner availability
YouTube-->>BannerComponent: Return banner details
LinkedIn-->>BannerComponent: Return banner details
BannerComponent-->>AnnouncementHero: Return visible banners
AnnouncementHero-->>User: Display updated banners
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for asyncapi-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3312--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: 0
🧹 Outside diff range and nitpick comments (2)
components/campaigns/banners.ts (2)
18-18: LGTM! Consider using a constant for improved maintainability.The updated deadline aligns with the shift to an online conference. The date format is correct (ISO 8601).
Consider defining this date as a constant at the top of the file for easier maintenance:
const CFP_DEADLINE_PARIS = '2024-10-30T06:00:00Z';Then use it like this:
const cfpDeadlineParis = CFP_DEADLINE_PARIS;
23-29: LGTM! Consider updating thecityproperty for consistency.The updates accurately reflect the shift to an online conference and are consistent with the PR objectives.
For consistency, consider updating the
cityproperty to reflect both platforms:- city: 'YouTube', + city: 'YouTube & LinkedIn',This would align with the
dateLocationandeventNameproperties that mention both platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/campaigns/banners.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
components/campaigns/banners.ts (1)
18-29: Overall, the changes successfully update the banner for the online conference.The modifications effectively transform the banner from advertising an in-person event in Paris to an online conference on YouTube and LinkedIn. The deadline extension and updated event details are consistent with this change.
To ensure all references to the Paris event have been updated, please run the following script:
This will help identify any remaining references that might need updating.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3312 +/- ##
=======================================
Coverage 49.14% 49.14%
=======================================
Files 21 21
Lines 647 647
=======================================
Hits 318 318
Misses 329 329 ☔ View full report in Codecov by Sentry. |
components/campaigns/banners.ts
Outdated
| @@ -15,18 +15,18 @@ function shouldShowBanner(cfpDeadline: string) { | |||
| return true; | |||
| } | |||
|
|
|||
| const cfpDeadlineParis = '2024-10-12T06:00:00Z'; | |||
| const cfpDeadlineParis = '2024-10-30T06:00:00Z'; | |||
There was a problem hiding this comment.
Instead of using this variable, can you please add this inside map? It is very confusing to understand why are we giving cfpDeadlineParis deadline to every banner 😅
There was a problem hiding this comment.
I'm lost @akshatnema please explain. Oooh I used default cfpparis because it was a quick change after the Paris banner 🫣🫣🫣
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
components/campaigns/banners.ts (1)
Line range hint
6-15: Fix typo and enhance date handling robustness.There's a typo in the comment "G et" and the date handling could be more robust.
Consider applying these improvements:
export function shouldShowBanner(cfpDeadline: string) { - const currentDate = new Date(); // G et the current date - const deadline = new Date(cfpDeadline); // Convert the cfpDeadline string to a Date object + const currentDate = new Date(); // Get the current date in UTC + + // Validate and parse the deadline + if (!cfpDeadline) { + throw new Error('cfpDeadline is required'); + } + + const deadline = new Date(cfpDeadline); + if (isNaN(deadline.getTime())) { + throw new Error('Invalid date format for cfpDeadline'); + } // Check if the current date is after the deadline if (currentDate > deadline) { return false; } return true; }components/campaigns/AnnouncementHero.tsx (2)
Line range hint
67-81: Add accessibility attributes to improve user experience.While the accessibility score is good (98), there's room for improvement:
- Add ARIA labels to navigation buttons
- Consider pausing auto-rotation on hover/focus
- Add keyboard navigation support
// In the navigation buttons section: <div className={`absolute left-0 ...`} onClick={goToPrevious} + aria-label="Previous banner" + role="button" + tabIndex={0} + onKeyDown={(e) => e.key === 'Enter' && goToPrevious()} > // In the useEffect: useEffect(() => { + const [isHovered, setIsHovered] = useState(false); - const interval = setInterval(() => setActiveIndex((index) => index + 1), 5000); + const interval = !isHovered && setInterval(() => setActiveIndex((index) => index + 1), 5000); return () => { clearInterval(interval); }; - }, [activeIndex]); + }, [activeIndex, isHovered]);
Line range hint
37-43: Consider implementing advanced carousel optimizations.The current implementation could benefit from some architectural improvements:
- Use
document.visibilityStateto pause rotation when the page is not visible- Reset
activeIndexwhen it grows too large- Consider using CSS transforms for smooth transitions
useEffect(() => { + const handleVisibilityChange = () => { + if (document.hidden) { + clearInterval(interval); + } else { + interval = setInterval(() => setActiveIndex((index) => index + 1), 5000); + } + }; + + document.addEventListener('visibilitychange', handleVisibilityChange); const interval = setInterval(() => { + setActiveIndex((index) => { + // Reset to prevent potential integer overflow + if (index >= Number.MAX_SAFE_INTEGER - len) return 0; + return index + 1; + }); - setActiveIndex((index) => index + 1) }, 5000); return () => { clearInterval(interval); + document.removeEventListener('visibilitychange', handleVisibilityChange); }; }, [activeIndex]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/campaigns/AnnouncementHero.tsx (3 hunks)
- components/campaigns/banners.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
components/campaigns/banners.ts (1)
20-26: Review the banner configuration for potential issues.Several concerns about the current implementation:
- The YouTube link contains tracking parameters which could change
- Using cfpDeadline for a live stream event might be confusing
- The hard-coded YouTube URL could be problematic if the stream URL changes
Let's verify if the YouTube link is accessible:
Consider these improvements:
- Store only the video ID and construct the URL dynamically
- Rename cfpDeadline to something more appropriate like 'eventDateTime'
- Add validation for the event date/time
- Consider adding a fallback URL in case the stream changes
Would you like me to provide a code example implementing these improvements?
components/campaigns/AnnouncementHero.tsx (2)
7-7: LGTM: Import statement is correctly structured.
Line range hint
67-81: Optimize banner rendering for better performance.The low Lighthouse performance score (47) suggests optimization opportunities:
- Memoize the banner visibility check
- Add image loading optimizations to the Banner component
- Consider implementing virtual scrolling for multiple banners
Consider passing these additional props to the Banner component:
loading="lazy"for imagespriorityflag for the first visible bannerplaceholderfor progressive loadingLet's check if the Banner component handles images:
| @@ -22,7 +22,7 @@ export default function AnnouncementHero({ className = '', small = false }: IAnn | |||
| const [activeIndex, setActiveIndex] = useState(0); | |||
|
|
|||
| const len = banners.length; | |||
| const numberOfVisibleBanners = banners.filter((banner) => banner.show).length; | |||
| const numberOfVisibleBanners = banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)).length; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the visible banners calculation.
The current implementation recalculates visible banners on every render and calls shouldShowBanner() multiple times for the same banner (here and in the render loop). Consider using useMemo to optimize performance:
- const numberOfVisibleBanners = banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)).length;
+ const visibleBanners = useMemo(() =>
+ banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)),
+ [banners]
+ );
+ const numberOfVisibleBanners = visibleBanners.length;Then update the render loop to use visibleBanners instead of filtering again.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
components/campaigns/AnnouncementHero.tsx (2)
25-26: Add shouldShowBanner to useMemo dependencies.While the memoization implementation is good, the dependency array should include all external values used in the callback.
- const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), [banners]); + const visibleBanners = useMemo(() => banners.filter((banner) => shouldShowBanner(banner.cfpDeadline)), [banners, shouldShowBanner]);
Line range hint
40-45: Optimize performance to improve Lighthouse score.Given the low Lighthouse performance score (47), consider these optimizations:
- Clear the interval when the banner is not visible:
useEffect(() => { + if (numberOfVisibleBanners <= 1) return; const interval = setInterval(() => setActiveIndex((index) => index + 1), 5000); return () => clearInterval(interval); - }, [activeIndex]); + }, [activeIndex, numberOfVisibleBanners]);
- Lazy load the arrow icons:
-import ArrowLeft from '../icons/ArrowLeft'; -import ArrowRight from '../icons/ArrowRight'; +const ArrowLeft = React.lazy(() => import('../icons/ArrowLeft')); +const ArrowRight = React.lazy(() => import('../icons/ArrowRight'));
- Consider increasing the rotation interval from 5000ms to reduce unnecessary re-renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/campaigns/AnnouncementHero.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
components/campaigns/AnnouncementHero.tsx (1)
7-7: LGTM!The import statement is correctly structured, importing both the banners array and the shouldShowBanner function.
| {visibleBanners.map((banner, index) => ( | ||
| <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={index === activeIndex % len} | ||
| className={className} | ||
| small={small} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Fix index mismatch between visible banners and navigation.
There are several issues with the current implementation:
- The activeIndex calculation uses the total number of banners (len) but renders only visible banners, which can cause index mismatches
- The pagination dots still show all banners instead of just the visible ones
Here's how to fix these issues:
- const len = banners.length;
+ const len = visibleBanners.length;
// ... rest of the code ...
<div className='m-auto flex justify-center'>
- {banners.map((banner, index) => (
+ {visibleBanners.map((banner, index) => (
<div
key={index}
className={`mx-1 size-2 cursor-pointer rounded-full ${
activeIndex % len === index ? 'bg-primary-500' : 'bg-gray-300'
}`}
onClick={() => goToIndex(index)}
/>
))}
</div>Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
components/campaigns/AnnouncementHero.tsx (2)
Line range hint
40-45: Optimize interval timer for better performance.Given the low Lighthouse performance score (44), consider optimizing the interval timer by:
- Using
requestAnimationFrameinstead ofsetIntervalfor smoother animations- Pausing the timer when the component is not visible using the Intersection Observer API
useEffect(() => { - const interval = setInterval(() => setActiveIndex((index) => index + 1), 5000); + let rafId: number; + let lastTime = performance.now(); + + const animate = (currentTime: number) => { + if (currentTime - lastTime >= 5000) { + setActiveIndex(index => index + 1); + lastTime = currentTime; + } + rafId = requestAnimationFrame(animate); + }; + + // Create intersection observer + const observer = new IntersectionObserver( + (entries) => { + if (entries[0].isIntersecting) { + rafId = requestAnimationFrame(animate); + } else { + cancelAnimationFrame(rafId); + } + }, + { threshold: 0 } + ); + + // Start observing the container + const container = document.querySelector('.announcement-hero'); + if (container) { + observer.observe(container); + } return () => { - clearInterval(interval); + cancelAnimationFrame(rafId); + observer.disconnect(); }; }, [activeIndex]);Also, add the
announcement-heroclass to the container:-<Container as='section' padding='' className='text-center'> +<Container as='section' padding='' className='text-center announcement-hero'>
Line range hint
52-60: Enhance accessibility for navigation controls.While the accessibility score is good (98), the navigation arrows could be more accessible with proper ARIA labels and roles.
<div className={`absolute left-0 top-1/2 z-10 mb-2 flex size-8 -translate-y-1/2 cursor-pointer items-center justify-center rounded-full bg-primary-500 opacity-50 hover:bg-primary-600 md:opacity-100`} onClick={goToPrevious} + role="button" + aria-label="Previous banner" + tabIndex={0} + onKeyDown={(e) => e.key === 'Enter' && goToPrevious()} > // ... similar changes for the right arrow ... <div className={`absolute right-0 top-1/2 z-10 mb-2 size-8 -translate-y-1/2 cursor-pointer rounded-full bg-primary-500 opacity-50 hover:bg-primary-600 md:opacity-100`} onClick={goToNext} + role="button" + aria-label="Next banner" + tabIndex={0} + onKeyDown={(e) => e.key === 'Enter' && goToNext()} >Also applies to: 92-100
| cfpDeadline={banner.cfpDeadline} | ||
| link={banner.link} | ||
| city={banner.city} | ||
| activeBanner={index === activeIndex % len} |
There was a problem hiding this comment.
Fix undefined variable in activeBanner prop calculation.
The len variable is undefined. This should use numberOfVisibleBanners instead to correctly calculate the active banner index.
-activeBanner={index === activeIndex % len}
+activeBanner={index === activeIndex % numberOfVisibleBanners}📝 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.
| activeBanner={index === activeIndex % len} | |
| activeBanner={index === activeIndex % numberOfVisibleBanners} |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/campaigns/banners.ts (1)
Line range hint
6-15: Function implementation can be improvedThe function has several areas for improvement:
- The comment has a typo: "G et" should be "Get"
- The implementation can be simplified
- The timezone handling should be considered for the deadline comparison
Consider this improved implementation:
/** * @param {string} cfpDeadline - The deadline for the call for papers - * @returns Whether the banner should be shown + * @returns {boolean} true if current time is before the deadline, false otherwise * @description Check if the current date is after the deadline */ export function shouldShowBanner(cfpDeadline: string) { - const currentDate = new Date(); // G et the current date - const deadline = new Date(cfpDeadline); // Convert the cfpDeadline string to a Date object - - // Check if the current date is after the deadline - if (currentDate > deadline) { - return false; - } - - return true; + return new Date() < new Date(cfpDeadline); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/campaigns/AnnouncementHero.tsx (3 hunks)
- components/campaigns/banners.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
components/campaigns/banners.ts (1)
20-26: Review the banner configurationSeveral concerns about the current implementation:
- The YouTube link contains query parameters (
si=kPCqgUzqAKC0FaqJ) which might be session-specific or temporary. Consider using a more permanent link format.- The deadline (
2024-10-30T06:00:00Z) is set to the same day as the event, which might not give users enough notice that the event is starting.- Given the low Lighthouse performance score (46/100), consider lazy loading the banner or implementing a more performance-optimized solution.
Let's check if this YouTube video ID is valid:
Consider these improvements:
- Use a more reliable YouTube URL format:
https://www.youtube.com/watch?v=F9wHxd-v2f0- Set the deadline a few hours before the event start time to avoid confusion
- Consider implementing lazy loading for the banner component to improve performance
components/campaigns/AnnouncementHero.tsx (4)
7-7: LGTM: Import statement is correctly structured.The addition of
shouldShowBannerimport is clean and follows the module's organization pattern.
28-28: LGTM: Navigation logic correctly handles visible banners.The navigation functions properly handle the circular navigation using the correct number of visible banners.
Also applies to: 32-32
82-86: LGTM: Pagination correctly reflects visible banners.The pagination implementation properly handles:
- Showing dots only for visible banners
- Correct active state calculation
24-25: Verify the banner filtering implementation.The memoization implementation looks good, but let's verify the banner filtering logic across the codebase.
✅ Verification successful
Let me gather more information about the banner filtering implementation to ensure it's complete and correct.
I have enough information to generate the final response now. The implementation has been fully verified.
Banner filtering logic is correctly implemented
The
shouldShowBannerfunction implements a straightforward deadline-based visibility check:
- It properly converts both current date and deadline to Date objects
- Returns
falseif current date is after the deadline- Returns
trueotherwise, showing banners only for upcoming eventsThe function is used correctly in
AnnouncementHero.tsxwith the memoized filter, and theBannerinterface properly defines thecfpDeadlinefield as a string, matching the function's parameter type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the shouldShowBanner implementation and its usage # Test 1: Check the shouldShowBanner implementation echo "Checking shouldShowBanner implementation:" rg -A 5 "export function shouldShowBanner" # Test 2: Verify all usages of shouldShowBanner echo "Checking all usages of shouldShowBanner:" rg "shouldShowBanner"Length of output: 1080
Script:
#!/bin/bash # Let's check the complete implementation of shouldShowBanner and its return logic # Check the full implementation of banners.ts echo "Checking complete banners.ts implementation:" cat components/campaigns/banners.ts # Check the Banner type/interface definition echo -e "\nChecking Banner type definition:" rg -A 5 "type Banner|interface Banner"Length of output: 1531
| {visibleBanners.map((banner, index) => ( | ||
| <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={index === activeIndex % numberOfVisibleBanners} | ||
| className={className} | ||
| small={small} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing banner rendering performance.
While the implementation is functionally correct, the rendering of multiple banners might contribute to the low performance score (46) in the Lighthouse report. Consider these optimizations:
- Implement lazy loading for inactive banners
- Use CSS transforms for transitions instead of mounting/unmounting
Here's a suggested optimization:
<div className='relative flex min-h-72 w-full items-center justify-center overflow-hidden lg:h-[17rem] lg:w-[38rem]'>
- {visibleBanners.map((banner, index) => (
+ {visibleBanners.map((banner, index) => {
+ // Only render active banner and immediate neighbors
+ const isVisible = Math.abs(index - (activeIndex % numberOfVisibleBanners)) <= 1;
+ if (!isVisible) 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={index === activeIndex % numberOfVisibleBanners}
className={className}
small={small}
/>
- ))}
+ );
+ })}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Please create a new issue for this and let's not target this issue in this PR.
There was a problem hiding this comment.
@akshatnema, I've created a new issue to track this optimization: #3335.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as per @akshatnema's request
gh issue create \
--title "Optimize banner rendering performance to improve Lighthouse score" \
--body "In PR #3312 (https://github.com/asyncapi/website/pull/3312), optimizing the banner rendering performance was suggested to improve the Lighthouse performance score.
Reference comment: https://github.com/asyncapi/website/pull/3312#discussion_r1812653123
Requested by @akshatnema." \
--assignee "akshatnema"
Length of output: 452
|
/rtm |
Summary by CodeRabbit
shouldShowBannerfunction for improved rendering control.