Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: create banner for online conf #3312

Merged
merged 10 commits into from
Oct 27, 2024

Conversation

thulieblack
Copy link
Member

@thulieblack thulieblack commented Oct 21, 2024

Summary by CodeRabbit

  • New Features
    • Updated event banner details for the AsyncAPI Online Conference 2024, including:
      • Title change to "AsyncAPI Online Conference 2024"
      • Location updated to "YouTube"
      • Date and location revised to "30th of October, 2024 | YouTube & LinkedIn"
      • Call to action changed to "Join us Live"
      • Live streaming link provided for the event.
    • Enhanced banner visibility logic using the shouldShowBanner function for improved rendering control.

Copy link

coderabbitai bot commented Oct 21, 2024

Walkthrough

The changes involve updates to the shouldShowBanner function's visibility and documentation in components/campaigns/banners.ts, as well as modifications to the banners array. The cfpDeadlineParis variable is removed, and its value is replaced with a new deadline of '2024-10-30T06:00:00Z'. The banners array is significantly altered, including updates to the title, city, dateLocation, cfaText, eventName, and link, reflecting a transition from an in-person event in Paris to an online conference hosted on YouTube and LinkedIn. Additionally, the visibility logic in AnnouncementHero.tsx is updated to utilize the shouldShowBanner function.

Changes

File Change Summary
components/campaigns/banners.ts - Changed shouldShowBanner from private to public.
- Removed cfpDeadlineParis variable.
- Updated banners array:
- Title: "AsyncAPI Conf on Tour'24" → "AsyncAPI Online Conference'24"
- City: 'Paris' → 'YouTube'
- DateLocation: '3rd - 5th of December, 2024
components/campaigns/AnnouncementHero.tsx - Updated import statement to include shouldShowBanner.
- Changed logic for numberOfVisibleBanners to use shouldShowBanner instead of show property.

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
Loading

🐰 "In the land where banners gleam,
A conference shifts, a new dream!
From Paris streets to YouTube's light,
Join us online, it will be bright!
With LinkedIn too, we'll share the cheer,
Hop along, the event is near!" 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for asyncapi-website ready!

Name Link
🔨 Latest commit 63e6a9c
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/671e765d71086c000815e914
😎 Deploy Preview https://deploy-preview-3312--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Oct 21, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 42
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-3312--asyncapi-website.netlify.app/

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the city property for consistency.

The updates accurately reflect the shift to an online conference and are consistent with the PR objectives.

For consistency, consider updating the city property to reflect both platforms:

-    city: 'YouTube',
+    city: 'YouTube & LinkedIn',

This would align with the dateLocation and eventName properties that mention both platforms.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3a8f69a and 5a40ab8.

📒 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.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.14%. Comparing base (5c11eb0) to head (63e6a9c).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@@ -15,18 +15,18 @@ function shouldShowBanner(cfpDeadline: string) {
return true;
}

const cfpDeadlineParis = '2024-10-12T06:00:00Z';
const cfpDeadlineParis = '2024-10-30T06:00:00Z';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lost @akshatnema please explain. Oooh I used default cfpparis because it was a quick change after the Paris banner 🫣🫣🫣

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add ARIA labels to navigation buttons
  2. Consider pausing auto-rotation on hover/focus
  3. 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:

  1. Use document.visibilityState to pause rotation when the page is not visible
  2. Reset activeIndex when it grows too large
  3. 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

📥 Commits

Files that changed from the base of the PR and between 17b8b0d and ff8a6bd.

📒 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:

  1. The YouTube link contains tracking parameters which could change
  2. Using cfpDeadline for a live stream event might be confusing
  3. 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:

  1. Store only the video ID and construct the URL dynamically
  2. Rename cfpDeadline to something more appropriate like 'eventDateTime'
  3. Add validation for the event date/time
  4. 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:

  1. Memoize the banner visibility check
  2. Add image loading optimizations to the Banner component
  3. Consider implementing virtual scrolling for multiple banners

Consider passing these additional props to the Banner component:

  • loading="lazy" for images
  • priority flag for the first visible banner
  • placeholder for progressive loading

Let'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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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]);
  1. 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'));
  1. Consider increasing the rotation interval from 5000ms to reduce unnecessary re-renders.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ff8a6bd and bef2e01.

📒 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.

Comment on lines 66 to 80
{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}
/>
))}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix index mismatch between visible banners and navigation.

There are several issues with the current implementation:

  1. The activeIndex calculation uses the total number of banners (len) but renders only visible banners, which can cause index mismatches
  2. 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Using requestAnimationFrame instead of setInterval for smoother animations
  2. 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-hero class 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

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bef2e01 and 7ad3ff6.

📒 Files selected for processing (1)
  • components/campaigns/AnnouncementHero.tsx (3 hunks)
🧰 Additional context used

cfpDeadline={banner.cfpDeadline}
link={banner.link}
city={banner.city}
activeBanner={index === activeIndex % len}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
activeBanner={index === activeIndex % len}
activeBanner={index === activeIndex % numberOfVisibleBanners}

@akshatnema
Copy link
Member

@CodeRabbit full review

Copy link

coderabbitai bot commented Oct 23, 2024

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 improved

The function has several areas for improvement:

  1. The comment has a typo: "G et" should be "Get"
  2. The implementation can be simplified
  3. 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

📥 Commits

Files that changed from the base of the PR and between 62d6e9a and e408168.

📒 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 configuration

Several concerns about the current implementation:

  1. The YouTube link contains query parameters (si=kPCqgUzqAKC0FaqJ) which might be session-specific or temporary. Consider using a more permanent link format.
  2. 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.
  3. 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:

  1. Use a more reliable YouTube URL format: https://www.youtube.com/watch?v=F9wHxd-v2f0
  2. Set the deadline a few hours before the event start time to avoid confusion
  3. 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 shouldShowBanner import 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 shouldShowBanner function implements a straightforward deadline-based visibility check:

  • It properly converts both current date and deadline to Date objects
  • Returns false if current date is after the deadline
  • Returns true otherwise, showing banners only for upcoming events

The function is used correctly in AnnouncementHero.tsx with the memoized filter, and the Banner interface properly defines the cfpDeadline field 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

Comment on lines +65 to +79
{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}
/>
))}
Copy link

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Implement lazy loading for inactive banners
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a new issue for this and let's not target this issue in this PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit fd48c6a into asyncapi:master Oct 27, 2024
19 checks passed
@thulieblack thulieblack deleted the thulieblack-patch-2 branch November 4, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants