Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a new DateTime component for the UI library, enhancing date and time selection capabilities. The changes span multiple files across the project, including the addition of new React components such as Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
apps/engineering/content/design/components/date-time.example.tsx
Outdated
Show resolved
Hide resolved
|
…yed/unkey into eng-1608-calendar-component
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
internal/icons/src/index.ts (1)
Line range hint
1-37: Consider organizing exports alphabetically.The icon exports could be organized alphabetically to improve maintainability and make it easier to locate specific icons. This would be particularly helpful as the icon library grows.
-export * from "./icons/bars-filter"; -export * from "./icons/bolt"; -export * from "./icons/book-bookmark"; -export * from "./icons/calendar"; +export * from "./icons/bars-filter"; +export * from "./icons/bolt"; +export * from "./icons/book-bookmark"; +export * from "./icons/calendar"; +export * from "./icons/caret-right"; +export * from "./icons/caret-right-outline"; +export * from "./icons/check"; +export * from "./icons/chevron-expand-y"; // ... (remaining exports in alphabetical order)apps/engineering/content/design/components/date-time.example.tsx (1)
32-44: Simplify nested div structure.The nested divs with similar styling can be simplified. Consider using a single div with combined classes.
- <div className="flex flex-col w-full"> - <div className="w-full p-4 border border-1-gray-12 h-32"> - <div className="w-full "> + <div className="flex flex-col w-full p-4 border border-1-gray-12 h-32"> <p className="m-0 p-0"> Date Range: <span>{date?.from?.toLocaleDateString() ?? "no date"}</span> -{" "} <span>{date?.to?.toLocaleDateString() ?? "no date"}</span> </p> <p className="m-0 p-0"> Time Span: <span>{JSON.stringify(startTime)}</span> -{" "} <span>{JSON.stringify(endTime)}</span> </p> - </div> - </div>internal/ui/src/components/date-time/date-time.tsx (1)
84-93: Maintain consistent casing in display names.The display names use inconsistent casing.
DateTime.rootshould beDateTime.Rootto match the casing of other component display names.-DateTime.displayName = "DateTime.root"; +DateTime.displayName = "DateTime.Root";internal/ui/src/components/date-time/components/time-split.tsx (1)
49-66: Simplify time conflict resolution logic.The time conflict resolution logic is complex and hard to maintain. Consider breaking it down into smaller, more focused functions.
const resolveTimeConflict = (type: TimeInputType, newTime: TimeUnit, existingTime: TimeUnit): TimeUnit => { const comparison = compareTimeUnits(newTime, existingTime); if (type === "start" && comparison > 0) return newTime; if (type === "end" && comparison < 0) return newTime; return existingTime; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/engineering/content/design/components/date-time.example.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)internal/ui/src/components/date-time/components/time-split.tsx(1 hunks)internal/ui/src/components/date-time/date-time.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
internal/ui/src/components/date-time/date-time.tsx (2)
41-43:⚠️ Potential issueImplement minDate and maxDate validation.
The
minDateandmaxDateprops are defined but not passed to the context or used in the component.-function DateTime({ children, className, onChange }: DateTimeRootProps) { +function DateTime({ children, className, minDate, maxDate, onChange }: DateTimeRootProps) { // ... existing code ... return ( <DateTimeContext.Provider value={{ date, startTime, endTime, + minDateRange: minDate, + maxDateRange: maxDate, onDateChange: handleDateChange, onStartTimeChange: handleStartTimeChange, onEndTimeChange: handleEndTimeChange, }} >Likely invalid or redundant comment.
51-64:⚠️ Potential issueFix stale state in change handlers.
The handlers are using potentially stale state values in the onChange callback due to closure. Use the new values instead.
const handleDateChange = (newRange: DateRange) => { setDate(newRange); - onChange(newRange, startTime, endTime); + onChange(newRange, startTime, endTime); }; const handleStartTimeChange = (newTime: TimeUnit) => { setStartTime(newTime); - onChange(date, newTime, endTime); + onChange(date, newTime, endTime); }; const handleEndTimeChange = (newTime: TimeUnit) => { setEndTime(newTime); - onChange(date, startTime, newTime); + onChange(date, startTime, newTime); };Likely invalid or redundant comment.
internal/ui/src/components/date-time/components/time-split.tsx (1)
137-138: Implement AM/PM and timezone support.The comment indicates that AM/PM and timezone support are pending implementation. This could affect the component's usability in different regions.
Would you like me to help implement the AM/PM and timezone support? This would involve:
- Adding AM/PM toggle
- Timezone selection
- Time conversion utilities
apps/engineering/content/design/components/date-time.example.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/icons/src/index.ts (1)
12-13:⚠️ Potential issueRemove duplicate export of circle-info-sparkle icon.
The
circle-info-sparkleicon is exported twice. This duplication should be removed.export * from "./icons/circle-info-sparkle"; -export * from "./icons/circle-info-sparkle";internal/ui/src/components/date-time/date-time.tsx (1)
52-65:⚠️ Potential issueFix state updates in change handlers.
The handlers are using stale state values in the onChange callback.
Apply this diff to fix the state updates:
const handleDateChange = (newRange: DateRange) => { setDate(newRange); - onChange(date, startTime, endTime); + onChange(newRange, startTime, endTime); }; const handleStartTimeChange = (newTime: TimeUnit) => { setStartTime(newTime); - onChange(date, startTime, endTime); + onChange(date, newTime, endTime); }; const handleEndTimeChange = (newTime: TimeUnit) => { setEndTime(newTime); - onChange(date, startTime, endTime); + onChange(date, startTime, newTime); };
🧹 Nitpick comments (2)
internal/icons/src/index.ts (1)
1-35: Consider alphabetically sorting exports for better maintainability.While the current organization works, alphabetically sorting the exports could help prevent future duplicate exports and make the file easier to maintain.
export * from "./icons/bars-filter"; export * from "./icons/bolt"; export * from "./icons/book-bookmark"; export * from "./icons/calendar"; export * from "./icons/caret-right"; export * from "./icons/caret-right-outline"; export * from "./icons/check"; export * from "./icons/chevron-expand-y"; export * from "./icons/chevron-left"; export * from "./icons/chevron-right"; export * from "./icons/circle-carret-right"; export * from "./icons/circle-info-sparkle"; -export * from "./icons/circle-info-sparkle"; export * from "./icons/fingerprint"; export * from "./icons/gauge"; export * from "./icons/gear"; export * from "./icons/grid"; export * from "./icons/input-search"; export * from "./icons/layers-3"; export * from "./icons/magnifier"; export * from "./icons/nodes"; export * from "./icons/plus"; export * from "./icons/refresh-3"; export * from "./icons/shield-check"; export * from "./icons/shield-key"; export * from "./icons/sliders"; export * from "./icons/sparkle-3"; export * from "./icons/task-checked"; export * from "./icons/task-unchecked"; export * from "./icons/time-clock"; export * from "./icons/trash"; export * from "./icons/triangle-warning"; export * from "./icons/triangle-warning-2"; export * from "./icons/ufo"; export * from "./icons/xmark";internal/ui/src/components/date-time/date-time.tsx (1)
33-33: Improve error message for context usage.The error message could be more descriptive to help developers understand how to fix the issue.
Apply this diff:
- throw new Error("DateTime components must be used within DateTime.Root"); + throw new Error("DateTime components must be wrapped within a <DateTime.Root> component. Please ensure the component is used within the DateTime context provider.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/engineering/content/design/components/date-time.example.tsx(1 hunks)internal/icons/src/index.ts(1 hunks)internal/ui/src/components/date-time/components/time-split.tsx(1 hunks)internal/ui/src/components/date-time/date-time.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/engineering/content/design/components/date-time.example.tsx
🧰 Additional context used
📓 Learnings (1)
internal/ui/src/components/date-time/components/time-split.tsx (2)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2810
File: internal/ui/src/components/date-time/components/time-split.tsx:0-0
Timestamp: 2025-01-22T16:52:29.277Z
Learning: For time input fields in React, use pattern="[0-9]*" for HTML5 validation and handle specific range validation (hours: 0-23, minutes/seconds: 0-59) in the onChange handler to ensure better user experience and consistent validation behavior.
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2810
File: internal/ui/src/components/date-time/components/time-split.tsx:10-14
Timestamp: 2025-01-22T16:51:59.978Z
Learning: The DateTime component in internal/ui/src/components/date-time/components/time-split.tsx already includes sufficient validation through handleChange and handleBlur functions, making additional runtime validation unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: autofix
🔇 Additional comments (4)
internal/icons/src/index.ts (1)
4-4: Well-structured icon additions for the datetime component.The new icons (
calendar,chevron-left,chevron-right,time-clock) provide a complete set of necessary icons for the datetime component implementation.Also applies to: 9-10, 30-30
internal/ui/src/components/date-time/date-time.tsx (2)
47-47:⚠️ Potential issueUse minDate and maxDate props in the component.
The props are defined but not destructured or passed to the context.
Apply this diff:
-function DateTime({ children, className, onChange }: DateTimeRootProps) { +function DateTime({ children, className, minDate, maxDate, onChange }: DateTimeRootProps) {And update the context provider:
value={{ date, startTime, endTime, + minDateRange: minDate, + maxDateRange: maxDate, onDateChange: handleDateChange, onStartTimeChange: handleStartTimeChange, onEndTimeChange: handleEndTimeChange, }}Likely invalid or redundant comment.
22-26: 🛠️ Refactor suggestionAdd type validation for TimeUnit.
The TimeUnit type allows any string values without validation, which could lead to invalid time inputs.
Apply this diff to add type validation:
export type TimeUnit = { - HH: string; - mm: string; - ss: string; + HH: `${number}${number}`; + mm: `${number}${number}`; + ss: `${number}${number}`; };Likely invalid or redundant comment.
internal/ui/src/components/date-time/components/time-split.tsx (1)
10-14: 🛠️ Refactor suggestionRemove duplicate TimeUnit type definition.
The TimeUnit type is already defined in the parent component. Consider importing it instead of redefining.
Apply this diff:
-export type TimeUnit = { - HH: string; - mm: string; - ss: string; -}; +import type { TimeUnit } from "../date-time";Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/ui/src/components/date-time/components/time-split.tsx (1)
49-66:⚠️ Potential issueFix incorrect time conflict resolution logic.
The time conflict resolution logic is incorrect. When start time is later than end time, it should update the end time to match start time, and vice versa.
const handleTimeConflicts = (normalizedTime: TimeUnit) => { if (date?.from && date.to) { if (!isSameDay(date.from, date?.to)) { return; } } if (type === "start" && endTime && compareTimeUnits(normalizedTime, endTime) > 0) { - onEndTimeChange(normalizedTime); + onStartTimeChange(normalizedTime); } else if (type === "end" && startTime && compareTimeUnits(normalizedTime, startTime) < 0) { - onStartTimeChange(normalizedTime); + onEndTimeChange(normalizedTime); } };
🧹 Nitpick comments (4)
internal/ui/src/components/date-time/components/time-split.tsx (4)
5-6: Consider removing the biome-ignore comment.Instead of ignoring the lint rule, consider importing only the necessary types from React to resolve the linting issue.
-// biome-ignore lint: React in this context is used throughout, so biome will change to types because no APIs are used even though React is needed. -import * as React from "react"; +import type { FC, FocusEvent } from "react";
10-14: Consider using number type for time values.The TimeUnit type currently uses strings for time values. Consider using numbers for better type safety and to avoid unnecessary string-to-number conversions.
export type TimeUnit = { - HH: string; - mm: string; - ss: string; + HH: number; + mm: number; + ss: number; };
141-142: Address TODO comment for AM/PM and timezone implementation.The comment indicates pending implementation for AM/PM and timezone features. Consider creating an issue to track this enhancement.
Would you like me to create an issue to track the implementation of AM/PM and timezone features?
150-162: Enhance accessibility for time range inputs.Add appropriate ARIA labels to improve accessibility when using range mode.
export const TimeInput: React.FC<TimeInputProps> = ({ type, className }) => { return ( <div + role="group" + aria-label={type === "range" ? "Time range selection" : "Time selection"} className={cn( "w-full h-full flex flex-row items-center justify-center gap-4 mt-2 px-1", className, )} > <TimeSplitInput type="start" /> {type === "range" ? <TimeSplitInput type="end" /> : null} </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ui/src/components/date-time/components/time-split.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/ui/src/components/date-time/components/time-split.tsx (2)
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2810
File: internal/ui/src/components/date-time/components/time-split.tsx:0-0
Timestamp: 2025-01-22T16:52:29.277Z
Learning: For time input fields in React, use pattern="[0-9]*" for HTML5 validation and handle specific range validation (hours: 0-23, minutes/seconds: 0-59) in the onChange handler to ensure better user experience and consistent validation behavior.
Learnt from: MichaelUnkey
PR: unkeyed/unkey#2810
File: internal/ui/src/components/date-time/components/time-split.tsx:10-14
Timestamp: 2025-01-22T16:51:59.978Z
Learning: The DateTime component in internal/ui/src/components/date-time/components/time-split.tsx already includes sufficient validation through handleChange and handleBlur functions, making additional runtime validation unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Icons
Documentation
Dependencies