feat(date-time-field): TEDI-Ready DateTimeField component #554#626
Open
airikej wants to merge 89 commits into
Open
feat(date-time-field): TEDI-Ready DateTimeField component #554#626airikej wants to merge 89 commits into
airikej wants to merge 89 commits into
Conversation
… feat/24-datefield-new-tedi-ready-component
… feat/24-datefield-new-tedi-ready-component
… feat/24-datefield-new-tedi-ready-component
… feat/24-datefield-new-tedi-ready-component
…54-datetimefield-component
…54-datetimefield-component
…/554-datetimefield-component
21 tasks
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces ChangesTime and DateTime Input Components
Supporting Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
airikej
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/tedi/components/content/calendar/calendar.module.scss (1)
66-83: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove duplicate property declarations for width and height.
Lines 69 and 71 declare
width: 100%andheight: 100%, but these are immediately overridden by lines 70 and 72 which set both properties tovar(--form-calendar-date-width). In CSS, only the last declaration takes effect, making the100%declarations dead code that adds confusion.🧹 Proposed fix to remove redundant declarations
button { all: unset; display: block; - width: 100%; width: var(--form-calendar-date-width); - height: 100%; height: var(--form-calendar-date-width); overflow: hidden;🤖 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/tedi/components/content/calendar/calendar.module.scss` around lines 66 - 83, The button rule in calendar.module.scss contains redundant width and height declarations (button selector has both width: 100%/height: 100% and then width: var(--form-calendar-date-width)/height: var(--form-calendar-date-width)); remove the initial width: 100% and height: 100% lines so only width: var(--form-calendar-date-width) and height: var(--form-calendar-date-width) remain in the button block (refer to the button selector and its focus-visible nested rule).src/tedi/components/form/date-field/date-field.tsx (2)
328-339: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the shared
getInitialMonthhelper to eliminate duplicate code.This local
getInitialMonthimplementation is identical to the exported helper indate-field-helpers.ts(lines 26-34). The refactor introduced the shared helper but didn't replace this usage, leaving duplicate logic.Import and use the helper version instead:
♻️ Refactor to use shared helper
Remove the local
getInitialMonthdefinition (lines 328-339) and update the import:import { buildDateRegexSource, CALENDAR_POPOVER_OFFSET, CALENDAR_POPOVER_PADDING, getLocaleDateParts, + getInitialMonth, } from './date-field-helpers';Then remove the
useCallbackwrapper at lines 328-339 and use the imported helper directly on lines 341, 345, etc.- const getInitialMonth = useCallback((val: Date | Date[] | DateRange | undefined, fallback?: Date): Date => { - if (val instanceof Date) return val; - - if (Array.isArray(val) && val.length > 0) { - return [...val].sort((a, b) => a.getTime() - b.getTime())[0]; - } - - if (val && typeof val === 'object' && 'from' in val && val.from instanceof Date) return val.from; - if (val && typeof val === 'object' && 'to' in val && val.to instanceof Date) return val.to; - - return fallback ?? new Date(); - }, []); - const [currentMonth, setCurrentMonth] = useState<Date>(() => getInitialMonth(value, initialMonth));The helper is a pure function with no dependencies, so wrapping it in
useCallbackprovides no benefit.🤖 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/tedi/components/form/date-field/date-field.tsx` around lines 328 - 339, The file defines a local getInitialMonth useCallback that duplicates the exported helper in date-field-helpers.ts; remove the local getInitialMonth definition and import the shared getInitialMonth from date-field-helpers.ts, then replace calls to the local wrapper with the imported pure function (no useCallback wrapper needed) in the DateField component (references to getInitialMonth in this file), ensuring any fallback argument is passed through as before.
396-411: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the shared
buildDisabledMatchershelper to eliminate duplicate code.This
useMemoblock duplicates the logic inbuildDisabledMatchersfromdate-field-helpers.ts(lines 57-72). The helper was created to be shared but is not being used here.Replace the inline matcher building with the helper:
♻️ Refactor to use shared helper
Update the import:
import { buildDateRegexSource, + buildDisabledMatchers, CALENDAR_POPOVER_OFFSET, CALENDAR_POPOVER_PADDING, getLocaleDateParts, } from './date-field-helpers';Replace lines 396-411:
- const disabledMatchers = useMemo<Matcher[]>(() => { - const matchers: Matcher[] = []; - - if (disabled) { - if (Array.isArray(disabled)) matchers.push(...disabled); - else matchers.push(disabled); - } - if (minDate) matchers.push({ before: minDate }); - if (maxDate) matchers.push({ after: maxDate }); - if (disablePast) matchers.push({ before: new Date() }); - if (disableFuture) matchers.push({ after: new Date() }); - if (shouldDisableMonth) matchers.push((date: Date) => shouldDisableMonth(date)); - if (shouldDisableYear) matchers.push((date: Date) => shouldDisableYear(date)); - - return matchers; - }, [disabled, minDate, maxDate, disablePast, disableFuture, shouldDisableMonth, shouldDisableYear]); + const disabledMatchers = useMemo<Matcher[]>( + () => buildDisabledMatchers({ disabled, minDate, maxDate, disablePast, disableFuture, shouldDisableMonth, shouldDisableYear }), + [disabled, minDate, maxDate, disablePast, disableFuture, shouldDisableMonth, shouldDisableYear] + );🤖 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/tedi/components/form/date-field/date-field.tsx` around lines 396 - 411, Replace the inline useMemo matcher construction with the shared helper buildDisabledMatchers: import buildDisabledMatchers and replace the const disabledMatchers = useMemo<Matcher[]>(() => { ... }, [...]) block by calling buildDisabledMatchers with the same arguments (disabled, minDate, maxDate, disablePast, disableFuture, shouldDisableMonth, shouldDisableYear) and assign its result to disabledMatchers (wrap in useMemo if you want to preserve memoization, using the same dependency list). This removes the duplicated logic and reuses the existing buildDisabledMatchers helper.
🧹 Nitpick comments (3)
src/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.spec.tsx (1)
129-142: ⚡ Quick winPrefer semantic-first element lookup in these tests.
Line 131 and Line 139 rely on
container.querySelector('.tedi-choice-group-item'), which tightly couples assertions to styling classes. Prefer semantic queries (e.g., fromgetByLabelText/getByRole) and only traverse to wrapper as a secondary step.As per coding guidelines, "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)."🤖 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/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.spec.tsx` around lines 129 - 142, Tests are using container.querySelector('.tedi-choice-group-item') which couples to styling; instead use semantic queries to find the input or card by role/label and then traverse to the wrapper if needed: in the two specs that call renderWithContext({ type: 'radio', variant: 'card' }) and renderWithContext({ type: 'checkbox', variant: 'card' }), use getByRole or getByLabelText to locate the radio/checkbox element (or the labelled card) and then find its closest parent element with class 'tedi-choice-group-item' to assert tabIndex, role, and aria-checked—this keeps the test semantic-first while still allowing wrapper assertions.src/tedi/components/overlays/dropdown/dropdown.spec.tsx (1)
198-217: ⚡ Quick winConsider adding a test case for omitted
defaultActiveIndex.The existing tests verify behavior when
defaultActiveIndexis explicitly set, but don't cover the case where it's omitted (undefined). Adding a test that verifiesfocusItemOnOpen: 'auto'behavior whendefaultActiveIndexis not provided would improve coverage and catch edge cases like the bug flagged in Line 175 of the implementation.📝 Suggested test case
it('uses auto focus behavior when defaultActiveIndex is omitted', () => { renderDropdown( { children: <span>Trigger</span> }, <> <Dropdown.Item index={0}>First</Dropdown.Item> <Dropdown.Item index={1}>Second</Dropdown.Item> </>, {} // No defaultActiveIndex provided ); fireEvent.click(screen.getByText('Trigger')); // When defaultActiveIndex is omitted, no item should be pre-focused // (aria-activedescendant should not be set initially) const menu = screen.getByRole('menu'); expect(menu).not.toHaveAttribute('aria-activedescendant'); });🤖 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/tedi/components/overlays/dropdown/dropdown.spec.tsx` around lines 198 - 217, Add a new test verifying behavior when defaultActiveIndex is omitted: render the dropdown via renderDropdown with no defaultActiveIndex, include two Dropdown.Item components (indexes 0 and 1), open the menu (fireEvent.click on the trigger) and assert that the menu element (getByRole('menu')) does NOT have aria-activedescendant set (i.e., no pre-focused item) to confirm focusItemOnOpen: 'auto' behavior when defaultActiveIndex is undefined.src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx (1)
51-58: ⚡ Quick winPrefer semantic assertions over test IDs in these cases.
getByTestId('row')/getByTestId('choice-group')can be replaced (or removed) in favor of role/text-based assertions already present in the test, which keeps the tests closer to user-observable behavior.As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)."Also applies to: 108-108
🤖 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/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx` around lines 51 - 58, Replace the non-semantic getByTestId assertions in the TimeGrid tests: instead of getByTestId('row') in the "renders grid variant" test and getByTestId('choice-group') in the "renders radio variant" test, assert the same UI using semantic queries (e.g., getAllByRole/getByRole or getByText) that reflect user-observable elements produced by the TimeGrid render call; update the tests named "renders grid variant" and "renders radio variant" (and the similar occurrence at the noted later line) to remove the testId checks and assert presence via roles/text such as the time buttons/list and the radiogroup/radios that are already being used in the expectations.
🤖 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/tedi/components/form/time-field/time-field.stories.tsx`:
- Around line 124-130: Two TimeField instances in the story share the same id
"calendar-button-trigger", breaking HTML uniqueness; update the second TimeField
(and any duplicates) to use a distinct id (e.g., "calendar-button-trigger-input"
or similar) and ensure any associated label/for or tests/query selectors
referencing the old id are updated to the new id so label associations and DOM
queries remain correct; locate the TimeField components in time-field.stories
(instances of <TimeField ... id="calendar-button-trigger" />) and change the
duplicate id value to a unique identifier.
In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx`:
- Around line 63-65: The selector interpolation in root.querySelector using the
variable value can throw a SyntaxError for special characters; update the
mount-time focus logic in time-grid.tsx to escape value before building the
selector (e.g., call CSS.escape(value) or a safe fallback) and use the escaped
string in the querySelector call (affecting the target assignment where
`input[type="radio"][value="${value}"], button[data-time="${value}"]` is built);
ensure CSS.escape is available or polyfilled and handle null/undefined value
cases before constructing the selector.
In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx`:
- Around line 265-316: The keyboard handler always computes currentIndex from
the external selected prop, so repeated Arrow/Home/End/Page keys don't move
relative to the last navigated item and Enter/Space picks the stale prop value;
fix handleColumnKeyDown by tracking the currently-focused index in a mutable ref
(e.g., focusedIndexRef) initialized from selected, use focusedIndexRef.current
instead of list.indexOf(selected) to compute currentIndex, update
focusedIndexRef.current = nextIndex after computing nextIndex, and when handling
Enter/Space call onSelect(list[focusedIndexRef.current]) so selection matches
the item the user navigated to; keep using hourRef/minuteRef and the existing id
pattern (`${uid}-${type}-${nextIndex}`) to focus/scroll elements.
In `@src/tedi/components/overlays/dropdown/dropdown.tsx`:
- Line 175: The null-check for focusItemOnOpen is wrong—replace the condition so
it only yields true when a numeric default index was actually provided; update
the expression that sets focusItemOnOpen (currently using defaultActiveIndex !==
null) to explicitly check the type (e.g., typeof defaultActiveIndex === 'number'
? true : 'auto') so undefined or non-number props do not force focus.
---
Outside diff comments:
In `@src/tedi/components/content/calendar/calendar.module.scss`:
- Around line 66-83: The button rule in calendar.module.scss contains redundant
width and height declarations (button selector has both width: 100%/height: 100%
and then width: var(--form-calendar-date-width)/height:
var(--form-calendar-date-width)); remove the initial width: 100% and height:
100% lines so only width: var(--form-calendar-date-width) and height:
var(--form-calendar-date-width) remain in the button block (refer to the button
selector and its focus-visible nested rule).
In `@src/tedi/components/form/date-field/date-field.tsx`:
- Around line 328-339: The file defines a local getInitialMonth useCallback that
duplicates the exported helper in date-field-helpers.ts; remove the local
getInitialMonth definition and import the shared getInitialMonth from
date-field-helpers.ts, then replace calls to the local wrapper with the imported
pure function (no useCallback wrapper needed) in the DateField component
(references to getInitialMonth in this file), ensuring any fallback argument is
passed through as before.
- Around line 396-411: Replace the inline useMemo matcher construction with the
shared helper buildDisabledMatchers: import buildDisabledMatchers and replace
the const disabledMatchers = useMemo<Matcher[]>(() => { ... }, [...]) block by
calling buildDisabledMatchers with the same arguments (disabled, minDate,
maxDate, disablePast, disableFuture, shouldDisableMonth, shouldDisableYear) and
assign its result to disabledMatchers (wrap in useMemo if you want to preserve
memoization, using the same dependency list). This removes the duplicated logic
and reuses the existing buildDisabledMatchers helper.
---
Nitpick comments:
In
`@src/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.spec.tsx`:
- Around line 129-142: Tests are using
container.querySelector('.tedi-choice-group-item') which couples to styling;
instead use semantic queries to find the input or card by role/label and then
traverse to the wrapper if needed: in the two specs that call
renderWithContext({ type: 'radio', variant: 'card' }) and renderWithContext({
type: 'checkbox', variant: 'card' }), use getByRole or getByLabelText to locate
the radio/checkbox element (or the labelled card) and then find its closest
parent element with class 'tedi-choice-group-item' to assert tabIndex, role, and
aria-checked—this keeps the test semantic-first while still allowing wrapper
assertions.
In
`@src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx`:
- Around line 51-58: Replace the non-semantic getByTestId assertions in the
TimeGrid tests: instead of getByTestId('row') in the "renders grid variant" test
and getByTestId('choice-group') in the "renders radio variant" test, assert the
same UI using semantic queries (e.g., getAllByRole/getByRole or getByText) that
reflect user-observable elements produced by the TimeGrid render call; update
the tests named "renders grid variant" and "renders radio variant" (and the
similar occurrence at the noted later line) to remove the testId checks and
assert presence via roles/text such as the time buttons/list and the
radiogroup/radios that are already being used in the expectations.
In `@src/tedi/components/overlays/dropdown/dropdown.spec.tsx`:
- Around line 198-217: Add a new test verifying behavior when defaultActiveIndex
is omitted: render the dropdown via renderDropdown with no defaultActiveIndex,
include two Dropdown.Item components (indexes 0 and 1), open the menu
(fireEvent.click on the trigger) and assert that the menu element
(getByRole('menu')) does NOT have aria-activedescendant set (i.e., no
pre-focused item) to confirm focusItemOnOpen: 'auto' behavior when
defaultActiveIndex is undefined.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6303f43a-d9f0-42cf-b31c-871bfb268beb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
skills/tedi-react/SKILL.mdskills/tedi-react/references/components.mdskills/tedi-react/references/forms.mdsrc/tedi/components/content/calendar/calendar.module.scsssrc/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.module.scsssrc/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.spec.tsxsrc/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.tsxsrc/tedi/components/form/date-field/date-field-helpers.tssrc/tedi/components/form/date-field/date-field.tsxsrc/tedi/components/form/date-time-field/date-time-field.module.scsssrc/tedi/components/form/date-time-field/date-time-field.spec.tsxsrc/tedi/components/form/date-time-field/date-time-field.stories.tsxsrc/tedi/components/form/date-time-field/date-time-field.tsxsrc/tedi/components/form/time-field/time-field-helpers.tssrc/tedi/components/form/time-field/time-field.module.scsssrc/tedi/components/form/time-field/time-field.spec.tsxsrc/tedi/components/form/time-field/time-field.stories.tsxsrc/tedi/components/form/time-field/time-field.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsxsrc/tedi/components/form/time-picker/time-picker.module.scsssrc/tedi/components/form/time-picker/time-picker.spec.tsxsrc/tedi/components/form/time-picker/time-picker.stories.tsxsrc/tedi/components/form/time-picker/time-picker.tsxsrc/tedi/components/overlays/dropdown/dropdown.spec.tsxsrc/tedi/components/overlays/dropdown/dropdown.tsxsrc/tedi/index.tssrc/tedi/providers/label-provider/labels-map.ts
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation