Chore/refactors and updates #83
Conversation
WalkthroughAdds style-prop passthroughs for dropdowns and datepicker, ref support to BigNumberField, validation and controlled state to FileUploader, exports Timeline types, a11y role to Tooltip, icon rendering gating by isLoading in ButtonIcon, minor layout tweaks, and new Storybook stories for dropdown select icons and FileUploader behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileUploader
participant Validator as validationFunction
participant Callback as onChange/callback
User->>FileUploader: Drop/select file
alt validationFunction provided
FileUploader->>Validator: validate(file)
alt valid
FileUploader->>FileUploader: set state (fileSelected) if uncontrolled
FileUploader-->>Callback: emit file
else invalid
FileUploader->>FileUploader: abort update
end
else no validator
FileUploader->>FileUploader: set state (fileSelected) if uncontrolled
FileUploader-->>Callback: emit file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/lib/dropdown/select/dropdown-container.tsx (1)
27-31: Use a unique id for ListBox (avoid hard-coded duplicates)Hard-coding id="listbox" will create duplicate IDs if multiple dropdowns render. Prefer a stable unique id via React.useId, or omit id unless referenced.
- <ListBox - id="listbox" + <ListBox + id={listBoxId}Add above the return:
const listBoxId = React.useId();src/lib/form/file-uploader.tsx (1)
24-24: Type-only nit: Omit syntax is incorrect (string literal used as a key union)fileTriggerProps?: Omit<FileTriggerProps, "acceptedFileTypes | onSelect"> uses a single string literal as the key, so neither prop is omitted. Use a union of keys.
- fileTriggerProps?: Omit<FileTriggerProps, "acceptedFileTypes | onSelect">; + fileTriggerProps?: Omit<FileTriggerProps, "acceptedFileTypes" | "onSelect">;
🧹 Nitpick comments (5)
src/lib/form/datepicker/index.tsx (2)
63-68: Prefer cn over clsx to tailwind-merge incoming popoverClassNameUsing cn here will resolve conflicting Tailwind classes (e.g., widths) when consumers pass popoverClassName.
- <Popover - className={clsx( + <Popover + className={cn( "bg-klerosUIComponentsWhiteBackground shadow-default rounded-base overflow-scroll", "border-klerosUIComponentsStroke ease-ease scrollbar border transition", time ? "w-82.5 lg:w-112.5" : "w-82.5", popoverClassName, - )} + )} >
71-77: Avoid duplicating width constraints on both Popover and inner wrapperWidth is applied to Popover and again to the inner wrapper, which can make overrides harder. Let the Popover own the width and keep the inner wrapper flexible.
- <div - className={clsx("flex", time ? "w-82.5 lg:w-112.5" : "w-82.5")} - > + <div className="flex"> <Calendar /> {time && <TimeControl {...{ minValue }} />} </div>src/stories/fileuploader.stories.tsx (1)
68-81: Pass the file argument to validationFunction (and minor copy nit).Use the file param to better illustrate the API and avoid type surprises. Also tweak the copy for clarity.
- msg: "This will not accept any file and invalidate", + msg: "This will mark any file as invalid.", - validationFunction: () => { - return false; - }, + validationFunction: (_file) => false,src/lib/form/bignumber-field/index.tsx (1)
20-21: Clarify ref naming and scope (optional).buttonRef only targets the increment button; that’s ambiguous. Consider incrementButtonRef (and optionally decrementButtonRef) for clarity and completeness.
- buttonRef?: React.Ref<HTMLButtonElement>; + /** Ref to the increment (up) button. */ + incrementButtonRef?: React.Ref<HTMLButtonElement>; + /** Optional ref to the decrement (down) button. */ + decrementButtonRef?: React.Ref<HTMLButtonElement>;And update destructuring/usages accordingly.
Also applies to: 36-38
src/lib/dropdown/select/index.tsx (1)
69-71: LGTM – className passthrough unlocks targeted styling.Passing dropdownClassName into DropdownContainer is clean and non-breaking.
Add a brief JSDoc for discoverability:
/** Extra className applied to the dropdown popover container. */ dropdownClassName?: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/button/ButtonIcon.tsx(1 hunks)src/lib/dropdown/cascader/dropdown-container.tsx(3 hunks)src/lib/dropdown/cascader/index.tsx(3 hunks)src/lib/dropdown/select/dropdown-container.tsx(1 hunks)src/lib/dropdown/select/index.tsx(3 hunks)src/lib/form/bignumber-field/index.tsx(4 hunks)src/lib/form/datepicker/index.tsx(3 hunks)src/lib/form/file-uploader.tsx(5 hunks)src/lib/progress/timeline/index.tsx(1 hunks)src/lib/tooltip/index.tsx(1 hunks)src/stories/dropdown-select.stories.tsx(2 hunks)src/stories/fileuploader.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T12:11:44.627Z
Learnt from: tractorss
PR: kleros/ui-components-library#80
File: src/lib/draggable-list/index.tsx:69-69
Timestamp: 2025-06-06T12:11:44.627Z
Learning: In React Aria's useDragAndDrop hook with ListBox, the keys passed to the getItems function represent items currently being dragged from the ListBox. These keys are guaranteed to exist, so using getItem(key)! with non-null assertion is safe and follows the documented pattern.
Applied to files:
src/lib/dropdown/select/dropdown-container.tsx
🧬 Code Graph Analysis (4)
src/lib/progress/timeline/index.tsx (2)
src/lib/progress/timeline/bullet.tsx (2)
SideProp(7-9)VariantProp(3-3)src/lib/progress/timeline/spine.tsx (1)
VariantProp(7-7)
src/lib/button/ButtonIcon.tsx (1)
src/utils/index.ts (1)
cn(4-6)
src/lib/dropdown/cascader/dropdown-container.tsx (1)
src/utils/index.ts (1)
cn(4-6)
src/lib/dropdown/select/dropdown-container.tsx (2)
src/lib/dropdown/select/item.tsx (1)
IItem(19-23)src/utils/index.ts (1)
cn(4-6)
🔇 Additional comments (7)
src/lib/dropdown/select/dropdown-container.tsx (1)
15-23: Good: className passthrough merged via cnMerging user className into Popover with cn is correct and consistent with the PR’s pattern.
src/lib/dropdown/cascader/index.tsx (1)
61-62: Good: forwards dropdownClassName to containerProp wiring is clear and matches other dropdown components. Enables targeted styling at the Popover/panel level.
src/lib/dropdown/cascader/dropdown-container.tsx (1)
83-91: Good: className passthrough merged via cn on PopoverCleanly merged and consistent with Select and DatePicker patterns.
src/lib/progress/timeline/index.tsx (1)
2-2: Verify VariantProp import sourceVariantProp appears to be exported from spine.tsx (not bullet.tsx). Ensure this import resolves, or switch to importing from "./spine" (or re-export it from bullet.tsx).
Suggested change if needed:
-import Bullet, { SideProp, VariantProp } from "./bullet"; +import Bullet, { SideProp } from "./bullet"; +import type { VariantProp } from "./spine";src/lib/form/file-uploader.tsx (2)
82-84: Validation flow is clearValidation short-circuits updates correctly before mutating state or invoking the callback. Good.
Also applies to: 95-97
124-124: Minor: message row alignment change looks fineitems-start → items-center improves vertical alignment for icon+text.
src/stories/dropdown-select.stories.tsx (1)
114-153: Story is solid; showcases per-item icons well.Nice, concise demo. Ensure IItem includes an optional icon?: React.ReactNode to align with usage (it likely does).
If not already present, add icon?: React.ReactNode to IItem.
|
✅ Deploy Preview for kleros-v2-ui-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
src/lib/progress/timeline/custom.tsx (2)
12-15: Forwarded props are untyped and aria-label overrides consumer input
- ICustomTimelineProps doesn’t extend DOM attributes, so ...props here is effectively empty from a typing perspective.
- aria-label is placed after {...props}, which forces "Timeline" and prevents consumers from overriding.
Fix:
- Extend React.OlHTMLAttributes so consumers can pass native attributes.
- Place {...props} last so consumers can override defaults.
-interface ICustomTimelineProps { +interface ICustomTimelineProps extends React.OlHTMLAttributes<HTMLOListElement> { items: [TimelineItem, ...TimelineItem[]]; className?: string; } @@ function CustomTimeline({ items, className, ...props }: Readonly<ICustomTimelineProps>) { @@ return ( <ol className={cn("box-border flex flex-col", className)} - {...props} aria-label="Timeline" + {...props} >Optional enhancement: if you want a default aria-label while still allowing override, compute it before render:
- const ariaLabel = props["aria-label"] ?? "Timeline";
- Then render aria-label={ariaLabel} and keep {...props} last.
Also applies to: 17-23, 25-29
5-10: Unify TimelineItem and strengthen CustomTimeline typingsThere are now two divergent
TimelineItemexports (inindex.tsxandcustom.tsx), which will confuse consumers and lead to broken imports. We should extract a single sharedTimelineItem(includingSideProp,StateProp, andVariantProp) into a new module (e.g.src/lib/progress/timeline/types.ts), update bothindex.tsxandcustom.tsxto import it, and then tighten up the props onCustomTimeline:• Consolidate
TimelineItemintosrc/lib/progress/timeline/types.tswith
– party:React.ReactNode
– extendsSideProp,StateProp, andVariantProp
• In bothindex.tsxandcustom.tsx, replace the localTimelineItemwith the shared one
• Incustom.tsx, updateICustomTimelinePropsto extendReact.OlHTMLAttributes<HTMLOListElement>
• Move the spread...propsafter the defaultaria-labelso consumers can override itApply these local changes in
src/lib/progress/timeline/custom.tsx:--- a/src/lib/progress/timeline/custom.tsx +++ b/src/lib/progress/timeline/custom.tsx @@ -import React from "react"; +import React from "react"; import Bullet, { StateProp, VariantProp } from "./bullet"; import { cn } from "../../../utils"; -export interface TimelineItem extends VariantProp, StateProp { +// ← Remove this local definition once you import the shared TimelineItem export interface TimelineItem extends VariantProp, StateProp { title: string; - party: string | React.ReactElement; + party: React.ReactNode; subtitle: string; Icon?: React.FC<React.SVGAttributes<SVGElement>>; } -interface ICustomTimelineProps { +interface ICustomTimelineProps extends React.OlHTMLAttributes<HTMLOListElement> { items: [TimelineItem, ...TimelineItem[]]; className?: string; } @@ return ( - <ol - className={cn("box-border flex flex-col", className)} - {...props} - aria-label="Timeline" + <ol + className={cn("box-border flex flex-col", className)} + aria-label="Timeline" + {...props} > {items.slice(0, -1).map((item, i) => ( <Bullet key={i} line {...item} rightSided isLast={false} />After extracting
TimelineItemintotypes.ts, import it here and inindex.tsx, then remove the local interface.src/lib/form/file-uploader.tsx (2)
24-24: Fix Omit usage: you're omitting a literal string, not a union of keysUse a union of property names so TypeScript actually omits both keys.
- fileTriggerProps?: Omit<FileTriggerProps, "acceptedFileTypes | onSelect">; + fileTriggerProps?: Omit<FileTriggerProps, "acceptedFileTypes" | "onSelect">;
94-112: PropagateisDisabledto FileTrigger and Button to fully disable interactionsCurrently only DropZone is disabled; file dialog can still open. Pass the flag down.
- <FileTrigger + <FileTrigger + isDisabled={isDisabled} acceptedFileTypes={acceptedFileTypes} {...fileTriggerProps} onSelect={(e) => { @@ - <Button + <Button + isDisabled={isDisabled} className={clsx( "box-border size-full cursor-pointer bg-transparent px-2", "flex items-center justify-center", )} >
🧹 Nitpick comments (5)
src/lib/progress/timeline/custom.tsx (2)
2-2: Import shared types from their source to reduce couplingVariantProp is defined in spine.tsx. Prefer importing it from there (or the new central types file), rather than via bullet.tsx. Keeps dependencies lean and avoids chained type re-exports.
-import Bullet, { StateProp, VariantProp } from "./bullet"; +import Bullet, { StateProp } from "./bullet"; +import type { VariantProp } from "./spine";
30-32: Avoid array index as a React keyIf items can be reordered/inserted, using the index as key can cause reconciliation issues. Prefer a stable unique key (e.g., an id field on TimelineItem). If not available, consider using a composite derived from stable fields.
src/lib/form/file-uploader.tsx (3)
28-31: TightenvalidationFunctiontype (param should not be optional)You always pass a File; making the param optional weakens the contract needlessly.
- /** Provide a custom validation function, returning false invalidates the file */ - validationFunction?: (file?: File) => boolean; + /** Provide a custom validation function; returning false prevents update and callback */ + validationFunction?: (file: File) => boolean;
86-89: DRY the validation/state/callback path across onDrop and onSelectBoth handlers duplicate the same steps. Factor into a small helper to reduce drift.
Example:
const acceptFile = (file: File) => { if (validationFunction && !validationFunction(file)) return; if (selectedFile === undefined) setFileSelected(file); callback(file); }; // onDrop: const file = await item.getFile(); acceptFile(file); // onSelect: const file = e[0]; acceptFile(file);Also applies to: 100-103
65-75: Handle wildcard and extension patterns in accepted types; prefer validating after reading fileincludes(item.type) won’t match "image/*" or ".png" patterns. Consider validating after getFile() using a matcher that supports MIME wildcards and extensions.
Example matcher:
function matchesAccepted(file: File, accepted?: Iterable<string>): boolean { if (!accepted) return true; const name = file.name.toLowerCase(); const type = file.type.toLowerCase(); for (const pat of accepted) { const p = pat.toLowerCase(); if (p === type) return true; // exact MIME if (p.endsWith("/*") && type.startsWith(p.slice(0, -1))) return true; // wildcard if (p.startsWith(".") && name.endsWith(p)) return true; // extension } return false; }Then:
const item = e.items.find((i): i is FileDropItem => i.kind === "file"); if (item) { const file = await item.getFile(); if (!matchesAccepted(file, acceptedFileTypes)) return; // continue… }Optionally adjust getDropOperation to return "copy" if types.has("file"), and do the stricter check after getFile().
Also applies to: 78-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/button/ButtonIcon.tsx(2 hunks)src/lib/form/file-uploader.tsx(6 hunks)src/lib/progress/timeline/custom.tsx(1 hunks)src/lib/progress/timeline/index.tsx(1 hunks)src/lib/tooltip/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/button/ButtonIcon.tsx
- src/lib/tooltip/index.tsx
- src/lib/progress/timeline/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/progress/timeline/custom.tsx (3)
src/lib/progress/timeline/index.tsx (1)
TimelineItem(6-10)src/lib/progress/timeline/bullet.tsx (2)
VariantProp(3-3)StateProp(11-13)src/lib/progress/timeline/spine.tsx (1)
VariantProp(7-7)
🔇 Additional comments (1)
src/lib/form/file-uploader.tsx (1)
47-54: Controlled/uncontrolled sync: LGTM and addresses prior feedbackState initializes from prop, syncs on change, and only writes when uncontrolled. This resolves the earlier review concern.
Changes:
inputRefandbuttonRefTimelineinterface exported from Timeline Componentitemprop's type in Timeline ComponentclassNamesto target popovers in Dropdown and DatePicker componentsvalidationFunctionand returning a boolean value to signify the validation.Bug fixes:
iconwhen Button is in loading statePR-Codex overview
This PR focuses on enhancing the functionality and usability of various components, including
Timeline,Dropdown,Button, andFileUploader, while adding new features like custom validation and controlled behavior for file uploads.Detailed summary
TimelineItemandTimelinePropstoexportincustom.tsxandindex.tsx.roleattribute to thedivinTooltipTrigger.dropdownClassNameprop inDropdownCascaderandDropdownSelect.ButtonIconreturn logic for handlingiconandisLoading.FileUploaderWithCustomValidationandFileUploaderWithControlledBehaviourstories.DropdownContainerandDatePickerto acceptclassNameandpopoverClassName.BigNumberFieldto acceptinputRefandbuttonRef.FileUploaderfor file selection.Summary by CodeRabbit
New Features
Bug Fixes
Documentation