feat: Implement carousel component with mock data#107
Open
BIA3IA wants to merge 3 commits into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR introduces a reusable carousel UI component built on Embla carousel, then integrates a mock carousel with sample content into the home page. The carousel supports keyboard navigation and dot-based pagination, with full state tracking and orientation support. ChangesCarousel Feature Implementation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/components/ui/carousel.tsx (2)
150-155: ⚡ Quick winUse
divinstead offieldsetfor carousel items.The
<fieldset>element is semantically meant for grouping form controls, not carousel slides. Using it here creates invalid HTML semantics and may confuse screen readers. Use a<div>instead, or<article>if each slide represents independent content.♻️ Proposed fix
return ( - <fieldset + <div aria-roledescription="slide" + role="group" data-slot="carousel-item" className={cn("min-w-0 shrink-0 grow-0 basis-full", orientation === "horizontal" ? "pl-4" : "pt-4", className)} {...props} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/carousel.tsx` around lines 150 - 155, The carousel slide element currently renders a <fieldset> (see the JSX that returns <fieldset aria-roledescription="slide" data-slot="carousel-item" className={cn(...)} {...props} />) which is semantically incorrect; replace that <fieldset> with a non-form container such as a <div> (or <article> if slides are independent content) while preserving all attributes (aria-roledescription, data-slot="carousel-item"), the className expression using cn(...), and spreading {...props} so behavior and styling remain unchanged.
125-125: ⚡ Quick winPrefer
onKeyDownoveronKeyDownCapture.Using
onKeyDownCaptureruns the handler before child components receive the event, which can interfere with keyboard handling in child elements (e.g., inputs, buttons). UseonKeyDowninstead to allow event bubbling and let children handle keyboard events first.♻️ Proposed change
<section - onKeyDownCapture={handleKeyDown} + onKeyDown={handleKeyDown} className={cn("relative", className)} data-slot="carousel" {...props}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/carousel.tsx` at line 125, Replace the use of the capturing keyboard handler with the regular bubbling handler: change the prop on the Carousel root (currently onKeyDownCapture={handleKeyDown}) to onKeyDown={handleKeyDown} so handleKeyDown will run during normal bubbling and allow child controls to receive key events first; update any related tests or comments referencing onKeyDownCapture to reflect the new onKeyDown prop and ensure handleKeyDown behavior remains correct.src/components/home/carousel-mock.tsx (1)
31-31: ⚖️ Poor tradeoffClarify the temporary nature of this component.
The TODO comment indicates this carousel mock should be deleted when merging. However, the PR title suggests adding a carousel component as a feature. Should this mock remain for demonstration purposes, or is it only for development/testing? If it's temporary, consider moving it to a separate demo route or Storybook story instead of the main home page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/home/carousel-mock.tsx` at line 31, The inline TODO "// TODO: delete this when merging" in carousel-mock.tsx is ambiguous—either make the mock explicitly temporary or move it out of the main Home flow; update the file to clarify intent and relocate usage: if this is temporary for development, change the TODO to a clear comment referencing the PR/issue and remove any import/use of the mock from the Home page (e.g., where Home imports CarouselMock); otherwise extract the component (named CarouselMock or the default export in carousel-mock.tsx) into a demo route (e.g., /demo/carousel) or a Storybook story under src/stories so the mock remains accessible without being shipped in the main page, and update the README or PR description to note where the demo lives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/home/carousel-mock.tsx`:
- Around line 17-31: The mockCards array contains extra blank lines inside the
object literals (e.g., the entries with title "WiFiLinux" and "The TOL Project")
causing Biome formatting errors; remove the stray blank lines inside each card
object so the properties are compact and then run pnpm exec biome check --write
to auto-fix and reformat the file (src/components/home/carousel-mock.tsx) and
commit the result.
In `@src/components/ui/carousel.tsx`:
- Around line 124-191: The file fails Biome formatting (extra spaces/line
breaks) in Carousel components; run the formatter or fix JSX spacing: run `pnpm
exec biome check --write` to auto-fix, or manually remove stray spaces and
incorrect line breaks around the CarouselContent, CarouselItem, and CarouselDots
JSX (e.g., stray spaces before closing tags, extra blank lines, and inconsistent
indentation) so the return blocks and component wrappers (CarouselContent,
CarouselItem, CarouselDots and their inner div/fieldset/Button elements) conform
to Biome formatting rules.
- Around line 98-107: The effect registers two Embla event listeners but only
removes one; in the useEffect that depends on api and onSelect, ensure you
unsubscribe both events by calling api.off("reInit", onSelect) and
api.off("select", onSelect) in the cleanup (mirror the two api.on calls). Update
the cleanup to reference the same handler (onSelect) for both "reInit" and
"select" so the reInit listener is removed when the component unmounts or api
changes.
- Line 174: The aria-label in the Carousel JSX is hardcoded in Italian ("Vai
alla slide ${index + 1}"); update it to use internationalization instead of a
hardcoded string—replace the inline Italian text in the aria-label attribute
with either an English default like `Go to slide ${index + 1}` or call your i18n
helper (e.g. t('carousel.go_to_slide', { index: index + 1 })) so the label is
localizable; locate the aria-label usage in the Carousel component (the JSX
containing aria-label={`Vai alla slide ${index + 1}`}) and switch it to the
translation helper or English fallback while preserving the dynamic index
interpolation.
- Around line 80-91: The handleKeyDown handler currently only handles
ArrowLeft/ArrowRight, breaking vertical carousels; update the handler to check
the carousel's orientation prop (e.g., orientation === "vertical") and when
vertical respond to ArrowUp/ArrowDown (calling scrollPrev/scrollNext and
event.preventDefault()), otherwise keep ArrowLeft/ArrowRight behavior; also
include orientation in the React.useCallback dependency array so the handler
updates when orientation changes.
---
Nitpick comments:
In `@src/components/home/carousel-mock.tsx`:
- Line 31: The inline TODO "// TODO: delete this when merging" in
carousel-mock.tsx is ambiguous—either make the mock explicitly temporary or move
it out of the main Home flow; update the file to clarify intent and relocate
usage: if this is temporary for development, change the TODO to a clear comment
referencing the PR/issue and remove any import/use of the mock from the Home
page (e.g., where Home imports CarouselMock); otherwise extract the component
(named CarouselMock or the default export in carousel-mock.tsx) into a demo
route (e.g., /demo/carousel) or a Storybook story under src/stories so the mock
remains accessible without being shipped in the main page, and update the README
or PR description to note where the demo lives.
In `@src/components/ui/carousel.tsx`:
- Around line 150-155: The carousel slide element currently renders a <fieldset>
(see the JSX that returns <fieldset aria-roledescription="slide"
data-slot="carousel-item" className={cn(...)} {...props} />) which is
semantically incorrect; replace that <fieldset> with a non-form container such
as a <div> (or <article> if slides are independent content) while preserving all
attributes (aria-roledescription, data-slot="carousel-item"), the className
expression using cn(...), and spreading {...props} so behavior and styling
remain unchanged.
- Line 125: Replace the use of the capturing keyboard handler with the regular
bubbling handler: change the prop on the Carousel root (currently
onKeyDownCapture={handleKeyDown}) to onKeyDown={handleKeyDown} so handleKeyDown
will run during normal bubbling and allow child controls to receive key events
first; update any related tests or comments referencing onKeyDownCapture to
reflect the new onKeyDown prop and ensure handleKeyDown behavior remains
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dbd7376-9f42-47de-8541-5717652e8a84
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.jsonsrc/app/page.tsxsrc/components/home/carousel-mock.tsxsrc/components/ui/carousel.tsx
BIA3IA
added
the
priority: high
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a carousel component along with mock data
Closes #104