UI/enhance datetime fields and allday slot #395
Conversation
WalkthroughRefactors DateTimeFields to use new read-only date field and editable time field components (ReadOnlyDateField, EditableTimeField), introduces slot-props helpers (getSlotProps, getFieldSlotProps, getTimeFieldSlotProps), and changes picker slotProps and layout. Duration/adjustment logic simplified so end-time updates no longer auto-adjust start-time in certain cases. Adds ReadOnlyPickerField component and EditableTimeField component files. Updates tests to use start/end test IDs, add ARIA label assertions, and add explicit blur waits. Adds optional Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/components/Event/components/DateTimeFields.tsx (3)
🔇 Additional comments (8)
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: 1
🧹 Nitpick comments (5)
src/components/Event/components/ReadOnlyPickerField.tsx (2)
19-26: Consider stronger typing for the validator function.Using
anyfor the generic picker props reduces type safety. While this is understandable for flexibility across date/time/datetime field types, consider whether you can leverage MUI's existing generic types or at least constrain the value type.
49-54: Empty string fallback for invalid dates may hide validation issues.When
pickerContext.value.isValid()returns false, the field displays an empty string. This could confuse users who entered partial input, as they won't see what they typed. Consider whether showing the raw input or a placeholder indicator would be more helpful for debugging UX issues.src/components/Event/components/DateTimeFields.tsx (3)
260-268: Consider consolidatinggetSlotPropsandgetFieldSlotPropsto reduce duplication.
getFieldSlotPropsduplicates most ofgetSlotProps().textField. Consider extracting shared properties into a base function to reduce maintenance overhead.+ const getBaseFieldProps = (testId: string, hasError = false) => ({ + size: "small" as const, + margin: "dense" as const, + fullWidth: true, + InputLabelProps: { shrink: true }, + error: hasError, + sx: { width: "100%" }, + inputProps: { "data-testid": testId }, + }); + const getSlotProps = (testId: string, hasError = false) => ({ - textField: { - size: "small" as const, - margin: "dense" as const, - fullWidth: true, - InputLabelProps: { shrink: true }, - error: hasError, - sx: { width: "100%" }, - inputProps: { "data-testid": testId }, - }, + textField: getBaseFieldProps(testId, hasError), }); - const getFieldSlotProps = (testId: string, hasError = false) => ({ - size: "small" as const, - margin: "dense" as const, - fullWidth: true, - InputLabelProps: { shrink: true }, - error: hasError, - sx: { width: "100%" }, - inputProps: { "data-testid": testId }, - }); + const getFieldSlotProps = getBaseFieldProps;
273-282: Fragile element detection usingtagName === "SPAN".Relying on the internal DOM structure of MUI X pickers (checking for SPAN elements) is brittle and may break with future MUI updates. Consider using a more robust approach such as checking for a specific class or data attribute, or simply opening on any click within the input container.
const handleTimeInputClick = ( event: React.MouseEvent, setOpen: (open: boolean) => void ) => { - // Only open if clicking on the text (span) - // In MUI X v6+, the sections are rendered as spans - if ((event.target as HTMLElement).tagName === "SPAN") { + const target = event.target as HTMLElement; + // Open if clicking on picker section content, not the icon button + if (target.closest('.MuiPickersSectionList-root')) { setOpen(true); } };
479-496: Inconsistent slotProps structure in condensed layout's end time picker.The end time picker in the condensed layout (lines 479-496) has a different structure than other time pickers:
openPickerButtonis placed aftertextFieldand the props are not using the spread pattern withgetSlotProps. Consider aligning the structure for consistency and maintainability.<TimePicker label={t("dateTimeFields.endTime")} ampm={false} value={endTimeValue} onChange={handleEndTimeChange} disabled={allday} open={isEndTimeOpen} onClose={() => setIsEndTimeOpen(false)} slotProps={{ - textField: { - size: "small", - margin: "dense" as const, - fullWidth: true, - InputLabelProps: { shrink: true }, - error: !!validation.errors.dateTime, - inputProps: { "data-testid": "end-time-input" }, - onClick: (e) => handleTimeInputClick(e, setIsEndTimeOpen), - sx: { - width: "100%", - "& .MuiPickersSectionList-section": { - cursor: "pointer", - }, - }, - }, - openPickerButton: { sx: { display: "none" } }, + ...getSlotProps("end-time-input", !!validation.errors.dateTime), + openPickerButton: { sx: { display: "none" } }, + textField: { + ...getSlotProps("end-time-input", !!validation.errors.dateTime).textField, + onClick: (e) => handleTimeInputClick(e, setIsEndTimeOpen), + sx: { + "& .MuiPickersSectionList-section": { + cursor: "pointer", + }, + }, + }, }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__test__/components/DateTimeField.test.tsx(4 hunks)src/components/Event/components/DateTimeFields.tsx(11 hunks)src/components/Event/components/ReadOnlyPickerField.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Event/components/DateTimeFields.tsx (1)
src/components/Event/components/ReadOnlyPickerField.tsx (1)
ReadOnlyDateField(85-89)
🔇 Additional comments (7)
src/components/Event/components/ReadOnlyPickerField.tsx (2)
75-75: Toggle behavior may cause unexpected UX.Using
setOpen((prev) => !prev)means clicking the field when the picker is already open will close it. While this might be intentional, typical date picker UX keeps the picker open until the user selects a date or clicks outside. Verify this is the desired behavior.
85-89: LGTM!Clean export of
ReadOnlyDateFieldwith proper prop typing and validator delegation.__test__/components/DateTimeField.test.tsx (4)
87-89: LGTM!Test interaction updated correctly to match the new ReadOnlyDateField behavior where clicking the input opens the picker.
131-133: LGTM!Consistent with other test updates for the end-date-input interaction.
152-154: LGTM!Correctly updated for allday event test scenario.
173-175: LGTM!Correctly updated for allday end-date interaction test.
src/components/Event/components/DateTimeFields.tsx (1)
322-337: LGTM on controlled time picker state.Good approach using controlled
openstate withonClosehandler. Hiding the picker button and opening on text click provides the intended UX.
|
no image was provided, built one : eriikaah/twake-calendar-front:issue-UI-enhance-datetime-fields-and-allday-slot |
|
Fixs for #373 right? Overall nice to me. |
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
2aca5b2 to
07ca916
Compare
|
Updated: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
__test__/components/EventModifications.test.tsx (1)
411-414: Consider usingwaitForinstead of hard-coded delay.The 150ms timeout is a timing workaround that could be brittle if the production delay changes. Consider using
waitForwith a state assertion instead:- // Wait for the 0.1s delay in handleStartTimeChangeWithClose to complete - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 150)); - }); + // Wait for the time change to be processed + await waitFor(() => { + expect(startDateInput).toHaveValue("08:00"); + });Alternatively, if the delay is unavoidable, consider extracting it as a named constant to make the coupling explicit and easier to maintain.
src/components/Calendar/TempCalendarsInput.tsx (1)
99-106: Compact PeopleSearch usage here looks appropriateHard‑coding
smallfor this temp‑calendar search keeps the UI compact and doesn’t affect logic. If TempCalendarsInput ever needs both compact and regular variants, you could expose asmall?: booleanprop and forward it instead, but that’s not required now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
__test__/components/DateTimeField.test.tsx(7 hunks)__test__/components/EventModifications.test.tsx(1 hunks)src/components/Attendees/AttendeeSearch.tsx(2 hunks)src/components/Attendees/PeopleSearch.tsx(2 hunks)src/components/Calendar/TempCalendarsInput.tsx(1 hunks)src/components/Event/EventFormFields.tsx(1 hunks)src/components/Event/components/DateTimeFields.tsx(6 hunks)src/components/Event/components/ReadOnlyPickerField.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Event/components/ReadOnlyPickerField.tsx
- src/components/Event/components/DateTimeFields.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Attendees/AttendeeSearch.tsx (1)
src/features/User/userDataTypes.ts (1)
userAttendee(37-44)
🔇 Additional comments (6)
__test__/components/DateTimeField.test.tsx (3)
87-87: LGTM! Test ID usage improves test stability.Switching from calendar button interactions to test ID-based selectors (
start-date-input,end-date-input) makes the tests more maintainable and less brittle to UI changes.Also applies to: 131-131, 152-152, 173-173
100-120: LGTM! Updated test reflects improved UX behavior.The test correctly validates that START date/time no longer auto-adjusts backward when END is changed to an earlier value. This is more intuitive user behavior - letting users create invalid ranges that they can then correct, rather than surprising them with automatic adjustments.
245-275: LGTM! Good accessibility test addition.The new test correctly verifies that all date/time inputs have
aria-labelattributes, which is essential for screen reader accessibility. All expected i18n keys (dateTimeFields.startDate,dateTimeFields.startTime,dateTimeFields.endDate,dateTimeFields.endTime) are properly defined in the locale files with meaningful translations that will be conveyed to screen readers at runtime.src/components/Event/EventFormFields.tsx (1)
523-531: PassingsmallintoAttendeeSelectoris consistent and non‑breakingWiring the new
smallprop here cleanly enables the compact attendee input without touching existing data flow or validation; the change is nicely scoped to presentation.src/components/Attendees/AttendeeSearch.tsx (1)
5-15: Clean pass‑through ofsmallto PeopleSearchAdding
small?: booleanhere and forwarding it to PeopleSearch cleanly exposes the compact variant to callers without touching the attendee mapping logic; existing usages that don’t passsmallkeep their prior behavior.Also applies to: 36-41
src/components/Attendees/PeopleSearch.tsx (1)
25-41:smallprop wiring keeps default behavior intactMaking
smalloptional and only using it to choose between"small"and"medium"on the input keeps existing callers' behavior unchanged while giving you a clean, explicit hook for compact rendering. The API surface remains simple and backwards compatible. MUI v7's TextField defaults to"medium"when the size prop is omitted, so the implementation correctly preserves this default for existing code.If you later need more than two sizes, you could consider generalizing this to a
size?: "small" | "medium"prop instead of a boolean, but that's purely future‑proofing.
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
07ca916 to
cdd4f20
Compare
Clicking here drived me off Expectation: open the time selector menu. As if I clicked the numbers. Actual: weird jumps. rec-oups-2025-12-08_10.59.19.mp4 |
Like Google calandar. |
Yes. Updating it to match exactly expected behavior |
…ropdown - Remove label prop from all DatePicker and TimePicker components - Add aria-label to inputProps for accessibility and testing - Auto-close TimePicker dropdown when selecting an option - Add 0.1s delay before calling time change handlers after closing dropdown - Add test case to verify aria-label attributes
- Add 150ms delay wait after time input change to ensure handler completes - Fixes test case that was failing due to 0.1s delay in handleStartTimeChangeWithClose
- Add separator '-' between start time and end time in normal mode - Fix layout logic: keep single row for allday events in normal mode - Remove auto-adjustment of start time when end time changes - Update test case to match new behavior
- Add small prop support to PeopleSearch component - Pass small prop to PeopleSearch in AttendeeSearch - Use small prop in EventFormFields Participants field - Use small prop in TempCalendarsInput for left sidebar
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
cdd4f20 to
5c8855d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Event/components/DateTimeFields.tsx (1)
230-281: Reduce duplication between slot helpers and removeas anyon field slot props
getFieldSlotPropsandgetTimeFieldSlotPropsare identical, and theslotProps.fieldusages are all cast withas any. SinceReadOnlyDateFieldis typed withDatePickerFieldPropsandEditableTimeFieldwithTimePickerFieldProps, you can:
- Introduce a shared, properly typed helper for field props (e.g., a generic over
DatePickerFieldProps/TimePickerFieldProps).- Reuse it for both date and time fields.
- Drop the
as anyassertions on allslotProps.fieldentries.This keeps the configuration (size, margin, aria-label, test IDs) in one place and restores type‑safety for MUI’s field slot props.
Also applies to: 303-377, 420-457, 463-557
🧹 Nitpick comments (3)
src/components/Event/components/DateTimeFields.tsx (1)
92-97: Boolean layout flags can be simplified
shouldShowFullFieldsInNormalis always equal toisExpanded, soshowSingleDateFieldeffectively reduces to!isExpanded && !shouldShowEndDateNormal. You can dropshouldShowFullFieldsInNormaland simplify this expression to make the layout logic easier to follow.- const isExpanded = showMore; - const shouldShowEndDateNormal = allday || showEndDate; - const shouldShowFullFieldsInNormal = isExpanded; - const showSingleDateField = - !isExpanded && !shouldShowEndDateNormal && !shouldShowFullFieldsInNormal; + const isExpanded = showMore; + const shouldShowEndDateNormal = allday || showEndDate; + const showSingleDateField = !isExpanded && !shouldShowEndDateNormal;src/components/Event/components/EditableTimeField.tsx (1)
122-161: Clarify 3‑digit time normalization behaviorThe 3‑digit normalization (
"123" → "1203"→ 12:03) is quite specific and will treat inputs like"930"as invalid (resetting to the previous time). If the intended UX is more typical “HMM → HH:MM” (e.g.930 → 09:30,945 → 09:45), you may want to adjust this logic accordingly, or at least add tests documenting the current behavior so it’s intentional rather than accidental.__test__/components/DateTimeField.test.tsx (1)
57-76: Align time-change tests with EditableTimeField’s commit behaviorThese tests change the time via
fireEvent.changeonstart-time-input/end-time-inputand then immediately expect the handlers to have been called. WithEditableTimeField, parsing andonChangeonly happen on blur/Enter (or via dropdown selection), not on every raw change event.To keep tests representative and robust, consider:
- Triggering a blur (
fireEvent.blur) or an Enter key press afterfireEvent.change, or- Switching to
userEvent.typefollowed by{enter}to better reflect real user interaction.Also applies to: 199-243
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
__test__/components/DateTimeField.test.tsx(7 hunks)__test__/components/EventModifications.test.tsx(1 hunks)src/components/Attendees/AttendeeSearch.tsx(2 hunks)src/components/Attendees/PeopleSearch.tsx(2 hunks)src/components/Calendar/CalendarSearch.tsx(1 hunks)src/components/Calendar/TempCalendarsInput.tsx(1 hunks)src/components/Event/EventFormFields.tsx(1 hunks)src/components/Event/components/DateTimeFields.tsx(6 hunks)src/components/Event/components/EditableTimeField.tsx(1 hunks)src/components/Event/components/ReadOnlyPickerField.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/Event/components/ReadOnlyPickerField.tsx
- src/components/Attendees/PeopleSearch.tsx
- src/components/Calendar/TempCalendarsInput.tsx
- src/components/Calendar/CalendarSearch.tsx
- test/components/EventModifications.test.tsx
- src/components/Event/EventFormFields.tsx
- src/components/Attendees/AttendeeSearch.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Event/components/DateTimeFields.tsx (3)
src/components/Event/utils/dateTimeFormatters.ts (1)
LONG_DATE_FORMAT(83-83)src/components/Event/components/ReadOnlyPickerField.tsx (1)
ReadOnlyDateField(84-88)src/components/Event/components/EditableTimeField.tsx (1)
EditableTimeField(259-267)
🔇 Additional comments (4)
src/components/Event/components/DateTimeFields.tsx (2)
102-210: Duration handling and “END before START” behavior look consistent with testsThe refactored handlers for start/end date and time correctly preserve the original duration when START moves, and only update the stored duration when END moves forward (while allowing END to go before START without auto-adjusting START). This aligns with the updated tests and the requested UX (“do not move START backward when END moves earlier”).
321-355: Time picker UI configuration matches the requested behaviorUsing
EditableTimeFieldas thefieldslot, hiding theopenPickerButton, forcing a single column withthresholdToRenderTimeInASingleColumn={48}, and constraining the popper width/alignments together should address the “click inside opens the menu and behaves like clicking the numbers, single column” feedback. The additional dash between start/end times in compact mode is a nice clarity touch.Also applies to: 380-413, 478-557
src/components/Event/components/EditableTimeField.tsx (1)
163-221: Event handling matches the desired interactive time picker behaviorClicking or focusing the field opens the picker and selects all text, blur/Enter commits parsed input, Escape reverts, and dropdown selections sync and close the popper cleanly. This should give the “click the time, type or pick, and have a single, stable menu” experience that was requested.
Also applies to: 223-256
__test__/components/DateTimeField.test.tsx (1)
245-275: ARIA-label accessibility test looks goodThe new test that asserts ARIA labels for start/end date and time on the inputs (using the mocked
treturning the key) correctly validates the accessibility contract introduced viainputProps["aria-label"].
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
5c8855d to
7ad4349
Compare
|
Updated: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Event/components/DateTimeFields.tsx (1)
230-281: Consolidate slot prop helpers and reduceas anyusage
getFieldSlotPropsandgetTimeFieldSlotPropsare currently identical (same keys and values), and both are later cast withas anywhen passed intoslotProps.field. You can:
- Collapse them into a single helper (e.g.,
getFieldSlotProps(testId, hasError, label)), or parameterize only the differences if you add any later.- Type that helper explicitly with the appropriate picker field prop types (e.g.,
DatePickerFieldProps/TimePickerFieldProps) so you can drop theas anycasts in theslotPropsspreads where these helpers are used.This keeps the slot configuration DRY and restores type‑safety around the custom field components.
🧹 Nitpick comments (2)
__test__/components/EventModifications.test.tsx (1)
412-415: Consider usingwaitForwith a condition instead of a fixed timeout.Fixed timeouts (like this 150ms delay) can make tests brittle—too short and they fail on slower systems; too long and they waste time. If the blur handler updates observable state or DOM, prefer waiting for that condition:
-// Wait for blur handler to complete -await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 150)); -}); +// Wait for blur handler to update state +await waitFor(() => { + expect(/* some observable change */).toBeTruthy(); +});If no observable condition exists (e.g., the blur handler only updates internal component state), the current approach may be acceptable, though fake timers could provide more control.
src/components/Event/components/DateTimeFields.tsx (1)
92-97: SimplifyshowSingleDateFieldcondition for clarity
shouldShowFullFieldsInNormalis always equal toisExpanded, so:const shouldShowFullFieldsInNormal = isExpanded; const showSingleDateField = !isExpanded && !shouldShowEndDateNormal && !shouldShowFullFieldsInNormal;is logically equivalent to:
const showSingleDateField = !isExpanded && !shouldShowEndDateNormal;You can drop the redundant flag in this expression to make the layout logic easier to read without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__test__/components/DateTimeField.test.tsx(10 hunks)__test__/components/EventModifications.test.tsx(1 hunks)src/components/Event/components/DateTimeFields.tsx(6 hunks)src/components/Event/components/EditableTimeField.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/components/DateTimeField.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Event/components/DateTimeFields.tsx (3)
src/components/Event/utils/dateTimeFormatters.ts (1)
LONG_DATE_FORMAT(83-83)src/components/Event/components/ReadOnlyPickerField.tsx (1)
ReadOnlyDateField(84-88)src/components/Event/components/EditableTimeField.tsx (1)
EditableTimeField(259-267)
🔇 Additional comments (6)
__test__/components/EventModifications.test.tsx (1)
409-410: Good addition of blur event to test realistic user interaction.Firing the blur event after the change event accurately simulates how users interact with the time input field (editing then tabbing away or clicking elsewhere). This ensures the blur handlers in the new datetime field components are properly tested.
src/components/Event/components/DateTimeFields.tsx (2)
196-210: End‑time duration update guard looks good; confirm end<start behavior is desiredThe new guard:
// Update duration when user changes end (if valid) if (!newEnd.isBefore(currentStart)) { initialDurationRef.current = newEnd.diff(currentStart, "minute"); }correctly avoids corrupting
initialDurationRefwhen the user picks an end time before the start. The handler then still callsonEndTimeChange(newTimeStr), so an invalid (end < start) interval is allowed and presumably surfaced viavalidation.errors.dateTimerather than auto‑correcting ashandleEndDateChangedoes.If that asymmetry (auto‑adjusting for date but not time) is intentional, this looks good; otherwise, you may want to mirror the end‑date behavior and adjust the start when the end time is moved before it.
298-559: Time picker slot wiring and layout customizations match the UX requirementsUsing
EditableTimeFieldin thefieldslot, hiding the defaultopenPickerButton, constraining the popper width, and settingthresholdToRenderTimeInASingleColumnplustimeSteps={{ minutes: 30 }}align with the requested “single column, click‑to‑open, 30‑minute step” behavior. The consistentdata-testidandaria-labelusage via the slot helpers is also good for accessibility and tests.Please just double‑check in the running UI that:
- Clicking anywhere in the start/end time fields reliably opens the dropdown.
- The digital clock renders as a single column with 30‑minute steps for all locales you support.
src/components/Event/components/EditableTimeField.tsx (3)
55-87: Value-sync and dropdown-closing behavior look solidThe combination of
getFormattedValue,inputValuestate, and the effect that:
- Updates
inputValuewhen the picker value changes, and- Closes the dropdown + blurs the input when a new value is selected while open,
should prevent the “weird jumps” and double-selection artifacts when interacting with the time dropdown. This is a good fit with the custom click-to-open behavior.
163-221: Click/focus/blur/keyboard handling seems well thought outThe handlers:
handleClickopening the picker and selecting the text,handleFocusselecting on focus without forcing the picker open,handleBlurskipping parsing when focus moves into the dropdown (usingrelatedTarget.closest(...)) and otherwise parsing + closing, andhandleKeyDowncommitting on Enter and reverting on Escape,give a consistent interaction model and should avoid the earlier “jumpy” behavior while still allowing direct typing. Nice balance between keyboard, mouse, and dropdown interactions.
223-255: MUI picker context integration matches the recommended patternUsing
useSplitFieldProps,usePickerContext, and wiring:
InputProps.ref = pickerContext.triggerRef,className,sx,ref, andnamefrompickerContext,matches MUI’s custom field pattern and keeps the field compatible with how pickers expect to manage focus and open state. Centering the input text via
& input { text-align: center }also makes sense for time entries.
|
Boolean layout comment looks good too. |
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
7ad4349 to
062901b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Event/components/DateTimeFields.tsx (1)
311-316: Type assertions remain despite prior review.The
as anyassertions on field slot props (lines 315, 350, 373, 408, 433, 453, 472, 507, 553) were flagged in a previous review and marked as addressed, but they're still present. While the prior review suggested importingDatePickerFieldPropsand properly typing the return value ofgetFieldSlotProps, the current implementation continues to suppress type checking.If typing challenges remain, consider documenting why the assertions are necessary or file a follow-up issue to properly type these props once MUI X type definitions stabilize.
Also applies to: 346-351, 369-374, 404-409, 429-434, 449-454, 468-473, 503-508, 549-554
🧹 Nitpick comments (1)
src/components/Event/components/DateTimeFields.tsx (1)
228-279: Consolidate duplicate helper functions.
getFieldSlotPropsandgetTimeFieldSlotProps(lines 247-262 and 264-279) are identical. Both return the same props structure with the same parameters. Consider merging them into a single function or eliminatinggetTimeFieldSlotPropsentirely and usinggetFieldSlotPropsthroughout.Additionally,
getSlotProps(lines 228-245) differs only by wrapping the result in atextFieldkey. You could refactor to a single base function that optionally wraps the result.Apply this diff to consolidate:
- const getFieldSlotProps = ( - testId: string, - hasError = false, - testLabel?: string - ) => ({ - size: "small" as const, - margin: "dense" as const, - fullWidth: true, - InputLabelProps: { shrink: true }, - error: hasError, - sx: { width: "100%" }, - inputProps: { - "data-testid": testId, - ...(testLabel ? { "aria-label": testLabel } : {}), - }, - }); - - const getTimeFieldSlotProps = ( + const getFieldSlotProps = ( testId: string, hasError = false, testLabel?: string ) => ({ size: "small" as const, margin: "dense" as const, fullWidth: true, InputLabelProps: { shrink: true }, error: hasError, sx: { width: "100%" }, inputProps: { "data-testid": testId, ...(testLabel ? { "aria-label": testLabel } : {}), }, });Then replace all
getTimeFieldSlotPropscalls withgetFieldSlotProps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__test__/components/DateTimeField.test.tsx(10 hunks)__test__/components/EventModifications.test.tsx(1 hunks)src/components/Event/components/DateTimeFields.tsx(6 hunks)src/components/Event/components/EditableTimeField.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/components/EventModifications.test.tsx
- test/components/DateTimeField.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Event/components/DateTimeFields.tsx (3)
src/components/Event/utils/dateTimeFormatters.ts (1)
LONG_DATE_FORMAT(83-83)src/components/Event/components/ReadOnlyPickerField.tsx (1)
ReadOnlyDateField(84-88)src/components/Event/components/EditableTimeField.tsx (1)
EditableTimeField(259-267)
🔇 Additional comments (8)
src/components/Event/components/DateTimeFields.tsx (3)
94-94: Cleaner logic for single-date-field display.Simplifying
showSingleDateFieldto depend only onisExpandedandshouldShowEndDateNormal(removing thespansMultipleDayscalculation) reduces complexity and improves readability.
511-521: Nice UX touch: visual separator between time fields.The dash separator between start and end time fields (lines 511-521) improves readability in the collapsed view. Conditional rendering based on
!alldayis appropriate since all-day events don't display times.
321-352: TimePicker configuration improvements align with PR objectives.The TimePicker changes across all instances effectively address the reported interaction bug:
- Using
EditableTimeFieldallows direct typing and opens the picker on click- Hiding
openPickerButtonremoves the need for a separate button click- Single-column layout (
thresholdToRenderTimeInASingleColumn={48}) with 30-minute steps provides better UX- Constrained popper width (110px) and left-aligned items improve visual consistency
These changes deliver the expected behavior described in PR comments: clicking the time field opens the selector menu, matching Google Calendar's interaction pattern.
Also applies to: 379-410, 477-509, 523-555
src/components/Event/components/EditableTimeField.tsx (5)
130-133: 3-digit normalization correctly implemented.The time parsing logic now uses
padStart(4, "0")for 3-digit inputs, which correctly handles cases like"830"→"0830"(08:30) and"123"→"0123"(01:23). This addresses the parsing issue flagged in the previous review.
32-33: Comprehensive time format support.The
TIME_PARSE_FORMATSarray (line 32) covers multiple input patterns ("HH:mm","H:mm","HHmm", etc.), providing flexibility for users who type times with or without colons and with 1- or 2-digit hours. This improves UX by accepting various common time entry patterns.
66-113: Complex but necessary synchronization logic.The dual
useEffecthooks managing input-value sync (lines 66-86) and dropdown open/close state (lines 88-113) handle the intricate coordination between the editable field and the picker dropdown. The logic correctly:
- Syncs input when picker value changes or field loses focus
- Closes dropdown after selection
- Refocuses input when dropdown opens to allow typing
The
setTimeoutcalls (lines 97-101) are a common pattern for deferring focus after DOM updates, though they can be fragile if timing assumptions change.
208-221: Standard keyboard interaction patterns.The keyboard handlers for Enter (commit/parse) and Escape (revert) follow standard input field conventions. Both appropriately close the dropdown and blur the input to ensure clean state transitions.
232-267: Clean component structure and public API.The
TextFieldrendering (lines 232-256) correctly wires up all necessary props, handlers, and refs from the picker context. The centered text alignment (lines 249-251) improves readability for time values.The exported
EditableTimeFieldwrapper (lines 259-267) provides a clean, typed public API that delegates to the internal generic implementation with time-specific configuration.
- Replace default TimePicker field with custom EditableTimeField - Allow free typing with multiple formats (HH:mm, HHmm, H, etc.) - Show dropdown on click while maintaining typing capability - Center-align time input text
lenhanphung
force-pushed
the
UI/enhance-datetime-fields-and-allday-slot
branch
from
062901b to
dbb669e
Compare
Summary by CodeRabbit
Tests
Refactor
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.