fix(ui): fix disabled date contrast and update datepicker tests (#28771)#28791
fix(ui): fix disabled date contrast and update datepicker tests (#28771)#28791nk10nikhil wants to merge 2 commits intocalcom:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughCalendar.tsx: adjusted 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/components/form/datepicker/datepicker.test.tsx (2)
59-63: Redundant assertion - already guaranteed by the filter.The assertion on line 63 is tautological:
disabledDatesis filtered to only include elements wherearia-disabled === "true"(line 60), so asserting that the first element has this attribute will always pass whendisabledDates.length > 0(which is already verified on line 62).Additionally,
awaitis unnecessary here sincetoHaveAttributeis synchronous.Consider simplifying:
Suggested fix
const disabledDates = getAllByRole("gridcell").filter( (cell) => cell.getAttribute("aria-disabled") === "true", ); expect(disabledDates.length).toBeGreaterThan(0); - await expect(disabledDates[0]).toHaveAttribute("aria-disabled", "true");If you want to keep an explicit assertion for documentation purposes, consider checking a property that isn't already guaranteed by the filter, or simply remove the redundant line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/components/form/datepicker/datepicker.test.tsx` around lines 59 - 63, The test builds disabledDates by filtering getAllByRole("gridcell") for cells with aria-disabled === "true", so the subsequent expect(disabledDates[0]).toHaveAttribute("aria-disabled", "true") is redundant and the await is unnecessary; remove that final assertion (or replace it with a non-tautological check such as verifying a different attribute or text on disabledDates[0])—update the test where disabledDates is defined and used to either drop the redundant expect/toHaveAttribute call or change it to assert something not already guaranteed by the filter.
43-43: Remove unnecessaryawait-toHaveAttributeis synchronous.The
toHaveAttributematcher from@testing-library/jest-domis synchronous and returns immediately. Theawaitis unnecessary here.Suggested fix
- await expect(selectedDate).not.toHaveAttribute("aria-disabled", "true"); + expect(selectedDate).not.toHaveAttribute("aria-disabled", "true");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/components/form/datepicker/datepicker.test.tsx` at line 43, The test uses an unnecessary await on the synchronous matcher toHaveAttribute; in the test file remove the await before the assertion so the line becomes a normal synchronous assertion on selectedDate (i.e., replace await expect(selectedDate).not.toHaveAttribute("aria-disabled", "true") with expect(selectedDate).not.toHaveAttribute("aria-disabled", "true")). Ensure other uses of toHaveAttribute in this test remain synchronous as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/components/form/datepicker/datepicker.test.tsx`:
- Around line 59-63: The test builds disabledDates by filtering
getAllByRole("gridcell") for cells with aria-disabled === "true", so the
subsequent expect(disabledDates[0]).toHaveAttribute("aria-disabled", "true") is
redundant and the await is unnecessary; remove that final assertion (or replace
it with a non-tautological check such as verifying a different attribute or text
on disabledDates[0])—update the test where disabledDates is defined and used to
either drop the redundant expect/toHaveAttribute call or change it to assert
something not already guaranteed by the filter.
- Line 43: The test uses an unnecessary await on the synchronous matcher
toHaveAttribute; in the test file remove the await before the assertion so the
line becomes a normal synchronous assertion on selectedDate (i.e., replace await
expect(selectedDate).not.toHaveAttribute("aria-disabled", "true") with
expect(selectedDate).not.toHaveAttribute("aria-disabled", "true")). Ensure other
uses of toHaveAttribute in this test remain synchronous as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f2116d5-f27c-4fc7-88b3-2eeac7bcea4c
📒 Files selected for processing (2)
packages/ui/components/form/date-range-picker/Calendar.tsxpackages/ui/components/form/datepicker/datepicker.test.tsx
There was a problem hiding this comment.
Pull request overview
This PR addresses WCAG 2.1 AA contrast compliance for disabled days in the UI date picker calendar (light mode), and updates the related tests to assert disabled state via accessibility attributes rather than styling.
Changes:
- Updated
react-day-pickerday_disabledstyling to use higher-contrast text colors in light mode. - Refactored DatePicker tests to identify disabled days via
aria-disabled="true"instead ofopacity-50. - Minor formatting adjustments in test renders/assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/ui/components/form/date-range-picker/Calendar.tsx | Updates disabled-day text color to improve contrast in light mode. |
| packages/ui/components/form/datepicker/datepicker.test.tsx | Updates tests to assert disabled state semantically via aria-disabled and removes style-based checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("Should show placeholder when no date is provided", () => { | ||
| const { getByTestId } = render(<DatePicker date={null as unknown as Date} />); | ||
| const { getByTestId } = render( | ||
| <DatePicker date={null as unknown as Date} />, | ||
| ); |
There was a problem hiding this comment.
The test passes null as unknown as Date into DatePicker, which bypasses the component’s date: Date prop typing. Consider either updating DatePicker to accept Date | null (and handle null when passing defaultMonth/selected to Calendar) or adjusting the test to avoid unsafe casts so it matches the supported API surface.
| @@ -36,28 +40,38 @@ describe("Tests for DatePicker Component", () => { | |||
| }); | |||
|
|
|||
| expect(selectedDate).toBeTruthy(); | |||
| await expect(selectedDate).not.toHaveClass("opacity-50"); | |||
| await expect(selectedDate).not.toHaveAttribute("aria-disabled", "true"); | |||
| }); | |||
There was a problem hiding this comment.
This test is named as if it validates date selection, but it never clicks a day or asserts that onDatesChange was called with the expected date. Instead it finds the cell with tabindex="0" (focus target) and asserts it’s not disabled, which doesn’t prove selection logic works. Update the test to click a specific enabled day and assert onDatesChange is invoked with that date (and optionally that the clicked day is not aria-disabled).
| const disabledDates = getAllByRole("gridcell").filter( | ||
| (cell) => cell.getAttribute("aria-disabled") === "true", | ||
| ); | ||
| expect(disabledDates.length).toBeGreaterThan(0); | ||
| await expect(disabledDates[0]).toHaveAttribute("disabled"); | ||
| await expect(disabledDates[0]).toHaveAttribute("aria-disabled", "true"); |
There was a problem hiding this comment.
In the minDate test, filtering cells by aria-disabled === "true" and then asserting the first result has aria-disabled="true" is redundant and doesn’t verify which dates are disabled. Consider asserting that a known date before minDate is disabled and that minDate itself is not disabled/selectable, to make the test meaningful and less dependent on internal rendering order.
| expect(dateButton.classList.toString()).toContain( | ||
| "disabled:cursor-not-allowed", | ||
| ); | ||
| expect(dateButton.classList.toString()).toContain("disabled:opacity-30"); |
There was a problem hiding this comment.
This test asserts that the rendered button’s class list contains Tailwind disabled:* variants, but those variants are present in the class string regardless of whether the button is actually disabled. To validate disabled={true} behavior, assert the trigger is disabled (e.g., toBeDisabled() / disabled attribute) and/or that clicking it does not open the calendar popover.
| expect(dateButton.classList.toString()).toContain( | |
| "disabled:cursor-not-allowed", | |
| ); | |
| expect(dateButton.classList.toString()).toContain("disabled:opacity-30"); | |
| expect(dateButton).toBeDisabled(); |
| day_outside: "", | ||
| day_disabled: "text-muted opacity-50", | ||
| day_range_middle: "aria-selected:bg-emphasis aria-selected:text-emphasis", | ||
| day_disabled: "text-[#4A4E59] dark:text-[#A3A3A3]", |
There was a problem hiding this comment.
day_disabled now uses hard-coded hex colors. This bypasses the project’s semantic theme tokens (e.g., text-subtle, text-muted) and makes the disabled-day color harder to tune for custom themes/branding. Consider introducing a semantic token/class for disabled date text (or using an existing token if one matches the required contrast) instead of embedding hex values here.
| day_disabled: "text-[#4A4E59] dark:text-[#A3A3A3]", | |
| day_disabled: "text-subtle", |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/components/form/datepicker/DatePicker.tsx (2)
10-11: Make callback nullability symmetric with thedateprop.
dateacceptsnull(Line 10), butonDatesChangeonly acceptsDate(Line 11). This makes controlled usage asymmetric and harder to model for “cleared” state.Proposed API-shape cleanup
type Props = { date: Date | null; - onDatesChange?: ((date: Date) => void) | undefined; + onDatesChange?: ((date: Date | null) => void) | undefined; className?: string; disabled?: boolean; minDate?: Date | null; label?: string; };Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/components/form/datepicker/DatePicker.tsx` around lines 10 - 11, The onDatesChange callback signature is asymmetric with the date prop: change the onDatesChange type so it accepts Date | null (matching the date prop) wherever declared (e.g., the onDatesChange prop in DatePicker.tsx and the other occurrence at the referenced line), update any related handler signatures (props and internal calls to onDatesChange) to pass null when the date is cleared, and adjust any TypeScript usages/calls to expect Date | null instead of only Date.
30-30: Renamecalendertocalendarfor readability.Small naming typo at Line 30 (used at Line 73) that hurts quick scanning.
Tiny readability diff
- const calender = ( + const calendar = ( <Calendar @@ - {calender} + {calendar}Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/components/form/datepicker/DatePicker.tsx` at line 30, Rename the misspelled variable "calender" to "calendar" throughout the DatePicker component: update the declaration (const calender = ...) and all usages (e.g., the JSX reference currently using calender) to "calendar" so the identifier is consistent and readable; ensure you update any related references in the same file to avoid TypeScript/JS errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/components/form/datepicker/DatePicker.tsx`:
- Around line 10-11: The onDatesChange callback signature is asymmetric with the
date prop: change the onDatesChange type so it accepts Date | null (matching the
date prop) wherever declared (e.g., the onDatesChange prop in DatePicker.tsx and
the other occurrence at the referenced line), update any related handler
signatures (props and internal calls to onDatesChange) to pass null when the
date is cleared, and adjust any TypeScript usages/calls to expect Date | null
instead of only Date.
- Line 30: Rename the misspelled variable "calender" to "calendar" throughout
the DatePicker component: update the declaration (const calender = ...) and all
usages (e.g., the JSX reference currently using calender) to "calendar" so the
identifier is consistent and readable; ensure you update any related references
in the same file to avoid TypeScript/JS errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 241706fe-6c79-4d3c-ad2e-a763bc766063
📒 Files selected for processing (2)
packages/ui/components/form/datepicker/DatePicker.tsxpackages/ui/components/form/datepicker/datepicker.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/components/form/datepicker/datepicker.test.tsx
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/ui/components/form/datepicker/DatePicker.tsx:32
- Minor naming issue: the local variable is named
calender(typo). Renaming tocalendarwould improve readability and reduce confusion when scanning the component.
const fromDate = minDate ?? new Date();
const calender = (
<Calendar
initialFocus
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| day: cn( | ||
| buttonClasses({ color: "minimal" }), | ||
| "w-8 h-8 md:h-11 md:w-11 p-0 text-sm font-medium aria-selected:opacity-100 inline-flex items-center justify-center" | ||
| "w-8 h-8 md:h-11 md:w-11 p-0 text-sm font-medium aria-selected:opacity-100 inline-flex items-center justify-center", | ||
| ), | ||
| day_range_end: "hover:bg-inverted! text-inverted!", | ||
| day_range_start: "hover:bg-inverted! text-inverted!", | ||
| day_selected: "bg-inverted text-inverted", | ||
| day_today: "", | ||
| day_outside: "", | ||
| day_disabled: "text-muted opacity-50", | ||
| day_range_middle: "aria-selected:bg-emphasis aria-selected:text-emphasis", | ||
| day_disabled: "text-[#4A4E59] dark:text-[#A3A3A3]", | ||
| day_range_middle: | ||
| "aria-selected:bg-emphasis aria-selected:text-emphasis", |
There was a problem hiding this comment.
day uses buttonClasses({ color: "minimal" }), which includes disabled:opacity-30 (see Button.tsx). Disabled days will therefore still render with reduced opacity even after changing day_disabled to a darker text color, likely reintroducing contrast failures. Consider overriding the disabled opacity for calendar day buttons (e.g., ensure disabled days render at full opacity while keeping a muted color) so the WCAG contrast fix is effective.
What does this PR do?
Changes:
text-muted opacity-50with accessible color values inCalendar.tsx:text-[#4A4E59]dark:text-[#A3A3A3](unchanged behavior, already compliant)aria-disabled="true"instead of relying onopacity-50Result:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Steps to reproduce (before fix):