Open
Conversation
Add takeover mdx file Add interactive examples
carlagn
requested review from
AmanVarshney01,
ArthurGamby,
aidankmcalister,
ankur-arch,
beseku,
mhartington and
mhessdev
Contributor
WalkthroughIntroduced a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![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 (2)
apps/eclipse/content/design-system/components/takeover.mdx (1)
7-18: Unused imports:AvatarandInputare imported but never used in this MDX file.These should be removed to keep imports clean.
import { Takeover, TakeoverMenu, TakeoverContent, TakeoverDescription, TakeoverHeader, TakeoverFooter, TakeoverTitle, Button, - Avatar, - Input, } from "@prisma/eclipse";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/eclipse/content/design-system/components/takeover.mdx` around lines 7 - 18, Remove the unused imports Avatar and Input from the import list that currently includes Takeover, TakeoverMenu, TakeoverContent, TakeoverDescription, TakeoverHeader, TakeoverFooter, TakeoverTitle, Button, Avatar, Input; update the import statement so it only imports the symbols actually used (e.g., Takeover, TakeoverMenu, TakeoverContent, TakeoverDescription, TakeoverHeader, TakeoverFooter, TakeoverTitle, Button) to eliminate the unused Avatar and Input imports.packages/eclipse/src/components/takeover.tsx (1)
4-4: Unused import:Buttonis imported but never used.This import should be removed to keep the module clean.
import { cva, type VariantProps } from "class-variance-authority"; import { cn } from "../lib/cn"; -import { Button } from "./button";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eclipse/src/components/takeover.tsx` at line 4, Remove the unused import of Button from takeover.tsx: delete the line importing Button (import { Button } from "./button";) so the module no longer imports an unused symbol and remains clean; verify there are no other references to Button in the takeover component and run the build/lint to confirm no missing usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/eclipse/content/design-system/components/takeover.mdx`:
- Around line 381-395: Update the Takeover documentation to remove or correct
references to non-existent media variants: delete the bullet "Two media variants
(default and icon)" from the Features list and remove or rewrite the sections
that mention "icon variant" and "default for larger visuals" as well as "Media
(icon variant)" and "Media (default variant)"; instead document the actual media
behavior of the Takeover component (or state that there is a single media
variant) and ensure all mentions reference the Takeover component by name so
readers aren’t misled by copy-pasta from the Empty component.
- Around line 211-213: The example in WizardWithFooter uses React.useState(1)
but does not import React; update the example to include a React import (or
import useState directly) so the component can run when copied—specifically add
an import for React or `useState` at the top of the snippet referenced by the
WizardWithFooter function so React.useState (or useState) is defined.
In `@packages/eclipse/src/components/takeover.tsx`:
- Line 22: The Tailwind class in the Takeover component's default class string
uses the invalid utility "align-center"; update the default class value (the
string containing "flex justify-between align-center") to use "items-center"
instead, i.e., replace "align-center" with "items-center" wherever that default
class string is defined in the Takeover component.
- Around line 50-55: Replace the non-semantic clickable <div>s that call onBack
and onClose (the elements with className "p-1.5" and icons) with proper <button
type="button"> elements to provide keyboard and screen-reader support; keep the
existing className and onClick handlers (onBack?.() and onClose?.()), add
descriptive aria-label attributes (e.g., "Back" and "Close"), and ensure any
visual focus styles are preserved so the Takeover component’s back and close
controls are keyboard-accessible and semantically correct.
- Around line 87-99: The TakeoverDescription component is typed as
React.ComponentProps<"p"> but returns a <div>, causing a prop/type mismatch;
update the component signature to use React.ComponentProps<"div"> (or
alternatively change the rendered element to <p>) so the props type matches the
rendered element (refer to TakeoverDescription and the React.ComponentProps<"p">
in the function declaration).
---
Nitpick comments:
In `@apps/eclipse/content/design-system/components/takeover.mdx`:
- Around line 7-18: Remove the unused imports Avatar and Input from the import
list that currently includes Takeover, TakeoverMenu, TakeoverContent,
TakeoverDescription, TakeoverHeader, TakeoverFooter, TakeoverTitle, Button,
Avatar, Input; update the import statement so it only imports the symbols
actually used (e.g., Takeover, TakeoverMenu, TakeoverContent,
TakeoverDescription, TakeoverHeader, TakeoverFooter, TakeoverTitle, Button) to
eliminate the unused Avatar and Input imports.
In `@packages/eclipse/src/components/takeover.tsx`:
- Line 4: Remove the unused import of Button from takeover.tsx: delete the line
importing Button (import { Button } from "./button";) so the module no longer
imports an unused symbol and remains clean; verify there are no other references
to Button in the takeover component and run the build/lint to confirm no missing
usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b17b93ba-b1be-410b-80de-5fad86d9a4c4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/eclipse/content/design-system/components/meta.jsonapps/eclipse/content/design-system/components/takeover.mdxapps/eclipse/src/components/takeover-examples/interactive-examples.tsxpackages/eclipse/src/components/index.tspackages/eclipse/src/components/takeover.tsx
Comment on lines
+211
to
+213
| export function WizardWithFooter() { | ||
| const [step, setStep] = React.useState(1); | ||
|
|
Contributor
There was a problem hiding this comment.
Missing React import in code example.
The code example uses React.useState(1) but doesn't import React. This will cause a runtime error if someone copies this code directly.
import {
Takeover,
TakeoverMenu,
TakeoverHeader,
TakeoverTitle,
TakeoverDescription,
TakeoverContent,
TakeoverFooter,
Button,
} from "@prisma/eclipse";
+import { useState } from "react";
export function WizardWithFooter() {
- const [step, setStep] = React.useState(1);
+ const [step, setStep] = useState(1);📝 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
| export function WizardWithFooter() { | |
| const [step, setStep] = React.useState(1); | |
| import { | |
| Takeover, | |
| TakeoverMenu, | |
| TakeoverHeader, | |
| TakeoverTitle, | |
| TakeoverDescription, | |
| TakeoverContent, | |
| TakeoverFooter, | |
| Button, | |
| } from "@prisma/eclipse"; | |
| import { useState } from "react"; | |
| export function WizardWithFooter() { | |
| const [step, setStep] = useState(1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/eclipse/content/design-system/components/takeover.mdx` around lines 211
- 213, The example in WizardWithFooter uses React.useState(1) but does not
import React; update the example to include a React import (or import useState
directly) so the component can run when copied—specifically add an import for
React or `useState` at the top of the snippet referenced by the WizardWithFooter
function so React.useState (or useState) is defined.
Comment on lines
+381
to
+395
| ### Features | ||
|
|
||
| - ✅ Flexible composition with semantic components | ||
| - ✅ TakeoverMenu with default and wizard variants | ||
| - ✅ TakeoverFooter for navigation and actions | ||
| - ✅ Two media variants (default and icon) | ||
| - ✅ Responsive design with proper spacing | ||
| - ✅ Automatic link styling in descriptions | ||
| - ✅ Centered text alignment | ||
| - ✅ Balanced text layout for readability | ||
| - ✅ Dashed border container | ||
| - ✅ Supports custom backgrounds and borders | ||
| - ✅ Icon sizing optimization | ||
| - ✅ Fully typed with TypeScript | ||
| - ✅ Customizable with className props |
Contributor
There was a problem hiding this comment.
Documentation mentions features that don't exist in the component.
The Features section lists "Two media variants (default and icon)" but the Takeover component doesn't have any media variants. This appears to be copy-pasted from the Empty component documentation.
Lines to review and potentially remove or update:
- Line 386: "Two media variants (default and icon)"
- Lines 403-404: mentions "icon variant" and "default for larger visuals"
- Lines 446-447: "Media (icon variant)" and "Media (default variant)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/eclipse/content/design-system/components/takeover.mdx` around lines 381
- 395, Update the Takeover documentation to remove or correct references to
non-existent media variants: delete the bullet "Two media variants (default and
icon)" from the Features list and remove or rewrite the sections that mention
"icon variant" and "default for larger visuals" as well as "Media (icon
variant)" and "Media (default variant)"; instead document the actual media
behavior of the Takeover component (or state that there is a single media
variant) and ensure all mentions reference the Takeover component by name so
readers aren’t misled by copy-pasta from the Empty component.
| const takeoverMenuVariants = cva("p-6 w-full", { | ||
| variants: { | ||
| variant: { | ||
| default: "flex justify-between align-center", |
Contributor
There was a problem hiding this comment.
Invalid Tailwind class: align-center should be items-center.
Tailwind CSS doesn't have an align-center utility. The correct class for vertical centering in a flex container is items-center.
- default: "flex justify-between align-center",
+ default: "flex justify-between items-center",📝 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
| default: "flex justify-between align-center", | |
| default: "flex justify-between items-center", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/takeover.tsx` at line 22, The Tailwind class
in the Takeover component's default class string uses the invalid utility
"align-center"; update the default class value (the string containing "flex
justify-between align-center") to use "items-center" instead, i.e., replace
"align-center" with "items-center" wherever that default class string is defined
in the Takeover component.
Comment on lines
+50
to
+55
| <div className="p-1.5" onClick={() => onBack?.()}> | ||
| <i className="text-foreground-neutral-weak fa-regular fa-arrow-left" /> | ||
| </div> | ||
| <div className="p-1.5" onClick={() => onClose?.()}> | ||
| <i className="text-foreground-neutral-weak fa-regular fa-xmark" /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Accessibility issue: Clickable <div> elements lack keyboard support and semantic roles.
These interactive elements are only clickable via mouse. Screen reader users and keyboard-only users won't be able to activate them. Consider using <button> elements instead, which provide built-in keyboard handling and proper semantics.
♿ Proposed fix using semantic buttons
- <div className="p-1.5" onClick={() => onBack?.()}>
- <i className="text-foreground-neutral-weak fa-regular fa-arrow-left" />
- </div>
- <div className="p-1.5" onClick={() => onClose?.()}>
- <i className="text-foreground-neutral-weak fa-regular fa-xmark" />
- </div>
+ <button
+ type="button"
+ className="p-1.5"
+ onClick={() => onBack?.()}
+ aria-label="Go back"
+ >
+ <i className="text-foreground-neutral-weak fa-regular fa-arrow-left" />
+ </button>
+ <button
+ type="button"
+ className="p-1.5"
+ onClick={() => onClose?.()}
+ aria-label="Close"
+ >
+ <i className="text-foreground-neutral-weak fa-regular fa-xmark" />
+ </button>📝 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
| <div className="p-1.5" onClick={() => onBack?.()}> | |
| <i className="text-foreground-neutral-weak fa-regular fa-arrow-left" /> | |
| </div> | |
| <div className="p-1.5" onClick={() => onClose?.()}> | |
| <i className="text-foreground-neutral-weak fa-regular fa-xmark" /> | |
| </div> | |
| <button | |
| type="button" | |
| className="p-1.5" | |
| onClick={() => onBack?.()} | |
| aria-label="Go back" | |
| > | |
| <i className="text-foreground-neutral-weak fa-regular fa-arrow-left" /> | |
| </button> | |
| <button | |
| type="button" | |
| className="p-1.5" | |
| onClick={() => onClose?.()} | |
| aria-label="Close" | |
| > | |
| <i className="text-foreground-neutral-weak fa-regular fa-xmark" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/takeover.tsx` around lines 50 - 55, Replace
the non-semantic clickable <div>s that call onBack and onClose (the elements
with className "p-1.5" and icons) with proper <button type="button"> elements to
provide keyboard and screen-reader support; keep the existing className and
onClick handlers (onBack?.() and onClose?.()), add descriptive aria-label
attributes (e.g., "Back" and "Close"), and ensure any visual focus styles are
preserved so the Takeover component’s back and close controls are
keyboard-accessible and semantically correct.
Comment on lines
+87
to
+99
| function TakeoverDescription({ | ||
| className, | ||
| ...props | ||
| }: React.ComponentProps<"p">) { | ||
| return ( | ||
| <div | ||
| data-slot="takeover-description" | ||
| className={cn( | ||
| "text-base text-foreground-neutral [&>a]:underline [&>a]:underline-offset-4 [&>a:hover]:text-primary mb-11! mt-0!", | ||
| className, | ||
| )} | ||
| {...props} | ||
| /> |
Contributor
There was a problem hiding this comment.
Type mismatch: TakeoverDescription is typed as ComponentProps<"p"> but renders a <div>.
The component accepts props for a <p> element but actually renders a <div>. This creates a type inconsistency. Either update the type to ComponentProps<"div"> or change the element to <p>.
function TakeoverDescription({
className,
...props
-}: React.ComponentProps<"p">) {
+}: React.ComponentProps<"div">) {
return (
<div🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/takeover.tsx` around lines 87 - 99, The
TakeoverDescription component is typed as React.ComponentProps<"p"> but returns
a <div>, causing a prop/type mismatch; update the component signature to use
React.ComponentProps<"div"> (or alternatively change the rendered element to
<p>) so the props type matches the rendered element (refer to
TakeoverDescription and the React.ComponentProps<"p"> in the function
declaration).
aidankmcalister
approved these changes
Mar 19, 2026
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
2 participants
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.
Summary by CodeRabbit