[FEAT] - Redesign Features section #79
Conversation
|
@AnthonyFinney is attempting to deploy a commit to the idan lodzki's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@AnthonyFinney has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces the feature grid with a slide-based carousel and refactors FeatureCard into a forwardRef two-column panel with new CTA/media props; adds Playfair Display and Source Sans 3 fonts and Tailwind font entries. Changes
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tailwind.config.js (1)
11-15: Font family additions are correct; consider dropping inner quotes for consistency
playfairandsource-sanswill work as-is, but you can simplify to['Playfair Display', 'serif']and `['Source Sans 3', 'sans-serif'] to match the existing Inter entries and Tailwind docs, and avoid embedding extra quotes in the generated CSS.styles/globals.css (1)
1-2: Global font import is aligned;next/fontcould be a future optimizationThe Google Fonts
@importcorrectly matches theplayfairandsource-sansfamilies defined in Tailwind. If you later want to tune performance and self-hosting/privacy, consider migrating these to Next.jsnext/font, but it’s not required for this PR.components/FeaturesSection.tsx (1)
137-159: Layout scaling effect is clever but a bit brittleThe
requestAnimationFrame+ manual padding/math to computeleftScaleandleftScaledHeightworks, but it’s fairly imperative and tied to the card’s exact padding and height (- 8adjustment, fixedmd/lgheights).If this area ever gets more dynamic (content/spacing changes), consider replacing this with a
ResizeObserveron the left content container, or relaxing the fixedmd:h/lg:hconstraints and letting the card height grow naturally. For now it’s fine, just something to watch.Also applies to: 171-179
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
public/gifs/demo.gifis excluded by!**/*.gifpublic/images/automatedActions.jpgis excluded by!**/*.jpgpublic/images/infrastructureManagement.jpgis excluded by!**/*.jpgpublic/images/logAggregation.jpgis excluded by!**/*.jpgpublic/images/openSource.jpgis excluded by!**/*.jpgpublic/images/real-timeMetrics.jpgis excluded by!**/*.jpgpublic/images/serviceDiscovery.jpgis excluded by!**/*.jpgpublic/images/smartAlerts.jpgis excluded by!**/*.jpgpublic/images/unifiedMonitoring.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
components/FeatureCard.tsx(1 hunks)components/FeaturesSection.tsx(1 hunks)styles/globals.css(2 hunks)tailwind.config.js(1 hunks)
🔇 Additional comments (1)
components/FeatureCard.tsx (1)
18-37: ForwardRefFeatureCardAPI and layout look solidThe refactor to a forwardRef-based, two-column panel with explicit
mediaand CTA props is clean and composes well with the FeaturesSection logic. External link handling viaisExternal+rel="noopener noreferrer"is also correct, and theleft*props give enough hooks for layout control without leaking implementation details.Also applies to: 47-81, 84-89
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/FeaturesSection.tsx (2)
214-241: Active slide indicator is visually de-emphasized when autoplay is offWhen
isPlayingisfalse(e.g., on mobile),progressstays0, so the active slide’s bar haswidth: 0%while all inactive bars render atwidth: 100%(just with lower opacity). That can make the active slide look “empty” compared to others.Consider making the active bar full-width when not playing so it’s still visually highlighted:
- style={{ width: slide === i ? `${Math.round(progress * 100)}%` : '100%' }} + style={{ + width: + slide === i + ? (isPlaying ? `${Math.round(progress * 100)}%` : '100%') + : '100%', + }}This keeps the animated fill behavior during autoplay while ensuring the active slide is clearly marked when playback is paused or disabled.
215-243: Extract repeated prev/next handlers for clarityThe inline
onClickhandlers for Previous and Next both perform the same sequence (setIsSlideChanging(true), update slide, reset progress,setTimeoutto clearisSlideChanging). Extracting these intogoToPrevious/goToNexthelpers would deduplicate logic and make the JSX easier to scan, without behavior changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/FeaturesSection.tsx(1 hunks)
🔇 Additional comments (3)
components/FeaturesSection.tsx (3)
27-102: Slides config and alt text look solidThe slide data is cohesive, and each
imageAltis now specific to the feature, which resolves the earlier generic-alt accessibility concern. No changes needed here.
104-121: Autoplay behavior and timer management look correctUsing
matchMedia('(min-width: 1024px)')to driveisPlayingcleanly disables autoplay on small screens while keeping it on for desktop, and the interval setup/cleanup avoids timer leaks. This also addresses the prior “no pause on mobile” UX issue by not auto-playing there.Also applies to: 126-146
155-177: Responsive left-column scaling effect is defensive and safeThe resize effect gracefully handles missing refs and guards against non-finite/negative scale values with a sane fallback to
1, which should keep the layout stable across resizes and different slide heights.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/FeaturesSection.tsx (3)
27-102: Static slide config is clear; consider lifting it outside the componentThe
slidesarray is fully static and wrapped inuseMemowith an empty dependency list, so its identity is already stable. You could optionally move this array to a top‑levelconst SLIDES: SlideSpec[] = [...]and just reference it inside the component to remove one hook and keep the render function a bit leaner. Not required, but it slightly simplifies the component structure.
126-165: Slide transition timeout could be centralized and cleaned up
isSlideChangingis flipped with asetTimeoutboth in the autoplay interval and inrunSlideTransition. That works, but it means there can be pending timeouts when the component unmounts or when users click quickly, which can trigger late state updates.If you want to tighten this up, consider:
- Storing the transition timeout handle in a
useRef<number | null>and clearing it in theuseEffectcleanup (alongside the intervals) and before scheduling a new timeout.- Optionally routing the autoplay advance through a single helper (e.g., a ref-backed
startTransitionfunction) that encapsulatessetIsSlideChanging,setSlide,setProgress(0), and the timeout.This would DRY up the transition logic and ensure no stray timeouts outlive the component.
226-263: Carousel controls are accessible; minor ARIA polish possibleThe prev/next and play/pause controls are generally accessible, and the slide indicators are keyboard‑focusable buttons. Two small, optional tweaks you might consider:
- For prev/next, you can drop the inner
span.sr-onlysincearia-labelalready provides the name; keeping both is redundant.- For the indicator buttons,
aria-label={Slide ${i + 1}}works, but you could make it more descriptive by including the feature name (e.g.,aria-label={\Go to feature: ${slides[i].heading}`}`) so screen reader users get more context.Pure polish; the current implementation is already usable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/FeaturesSection.tsx(1 hunks)
🔇 Additional comments (2)
components/FeaturesSection.tsx (2)
104-120: Viewport‑driven autoplay initialization looks solidNice use of
matchMediawith bothaddEventListenerandaddListenerbranches plus the initialsetIsPlaying(mediaQuery.matches). This cleanly disables autoplay on small screens while enabling it on desktop and keeps behavior in sync on resize. No changes needed here.
167-189: Layout scaling effect and cleanup look correctThe resize/layout effect correctly guards for missing refs, computes padding‑aware available height, and cleans up both the
requestAnimationFrameandresizelistener on dependency changes and unmount. Given it only toucheswindowinsideuseEffect, this should be safe for SSR/hydration as well. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/FeaturesSection.tsx (1)
165-187: Consider optimizing the scale calculation effect.The current implementation re-adds the resize listener every time the slide changes. While functional, this is inefficient. The
recomputefunction should run when the slide changes, but the listener itself only needs to be set up once.Refactor to separate the one-time listener setup from the slide-triggered recompute:
+ useEffect(() => { + const recompute = () => { + const hero = heroRef.current; + const inner = leftContentRef.current; + if (!hero || !inner) return; + const styles = window.getComputedStyle(hero); + const padT = parseFloat(styles.paddingTop || '0'); + const padB = parseFloat(styles.paddingBottom || '0'); + const availableH = hero.clientHeight - padT - padB - 8; + const neededH = inner.scrollHeight; + const sRaw = Math.min(1, availableH / neededH); + const s = Number.isFinite(sRaw) && sRaw > 0 ? sRaw : 1; + setLeftScale(s); + setLeftScaledHeight(Math.round(neededH * s)); + }; + recompute(); + }, [slide]); + useEffect(() => { const recompute = () => { const hero = heroRef.current; const inner = leftContentRef.current; if (!hero || !inner) return; const styles = window.getComputedStyle(hero); const padT = parseFloat(styles.paddingTop || '0'); const padB = parseFloat(styles.paddingBottom || '0'); const availableH = hero.clientHeight - padT - padB - 8; const neededH = inner.scrollHeight; const sRaw = Math.min(1, availableH / neededH); const s = Number.isFinite(sRaw) && sRaw > 0 ? sRaw : 1; setLeftScale(s); setLeftScaledHeight(Math.round(neededH * s)); }; - const id = requestAnimationFrame(recompute); - const onResize = () => recompute(); + const onResize = () => { + const id = requestAnimationFrame(recompute); + return () => cancelAnimationFrame(id); + }; window.addEventListener('resize', onResize); return () => { - cancelAnimationFrame(id); window.removeEventListener('resize', onResize); }; - }, [slide]); + }, []);This way the resize listener is registered once on mount, and the recompute logic in the separate effect runs whenever the slide changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/FeaturesSection.tsx(1 hunks)
🔇 Additional comments (2)
components/FeaturesSection.tsx (2)
102-118: Responsive autoplay implementation looks good.The media query effect properly addresses the past review concern by disabling autoplay on small screens. The fallback for older browser APIs (
addListener/removeListener) ensures broad compatibility, and cleanup is handled correctly for both paths.
189-264: Well-structured carousel controls and accessibility.The render implementation demonstrates good practices:
- Proper semantic HTML with accessibility features (aria-labels on all interactive controls)
- Decorative elements correctly marked with
aria-hidden- Responsive control visibility: play/pause hidden on small screens while keeping manual navigation available
- Progress indicators provide clear visual feedback
- Image optimization with
priorityfor first slide and propersizesattribute
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/FeaturesSection.tsx (1)
90-97: Consider more portable types for timer refsUsing
NodeJS.Timeoutin refs can be awkward in environments that combine DOM and Node typings (e.g., Next.js), wheresetInterval/setTimeoutmay be typed asnumber. To avoid type friction, you can lean onReturnType:- const timerRef = useRef<NodeJS.Timeout | null>(null); - const progressRef = useRef<NodeJS.Timeout | null>(null); - const transitionRef = useRef<NodeJS.Timeout | null>(null); + const timerRef = useRef<ReturnType<typeof setInterval> | null>(null); + const progressRef = useRef<ReturnType<typeof setInterval> | null>(null); + const transitionRef = useRef<ReturnType<typeof setTimeout> | null>(null);This stays correct regardless of whether the global timer functions resolve to
numberorTimeout.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/FeaturesSection.tsx(1 hunks)
🔇 Additional comments (2)
components/FeaturesSection.tsx (2)
15-88: Slide metadata and alt text look solidThe SLIDES definitions read well, have specific, non-redundant
imageAltvalues, and the descriptions/CTAs are consistent. This nicely addresses the earlier generic-alt concern while keeping the data structure clean and extensible.
103-147: Autoplay behavior and transition cleanup look goodThe media-query–driven
isPlayinginitialization and listener cleanly restrict autoplay tolg+ viewports, which addresses the earlier mobile-pause concern without exposing extra controls on small screens. The use oftransitionRefin both the autoplay interval andrunSlideTransitionensures slide transitions are debounced and that timeouts are not left dangling across transitions or unmounts.
1 participant
What Was Changed
Redesign Features section
Why Was It Changed
To make it look less generic.
Screenshots
Summary by CodeRabbit
New Features
Style