Skip to content

feat(DatePicker): add allowOnlyDatesList prop to DatePicker#994

Open
vickiwyang wants to merge 11 commits intomainfrom
bil-5134-add-allowlist-param-to-DatePicker
Open

feat(DatePicker): add allowOnlyDatesList prop to DatePicker#994
vickiwyang wants to merge 11 commits intomainfrom
bil-5134-add-allowlist-param-to-DatePicker

Conversation

@vickiwyang
Copy link
Copy Markdown

@vickiwyang vickiwyang commented Apr 14, 2026

Why?

  • @ClickHouse/billing-eng is using the DatePicker component to allow users to select a new bill date for an organization. Only some dates are eligible, and we'd like to be able to disable all non-eligible dates in the UI.

How?

  • Adds optional allowOnlyDatesList prop to DatePicker: when provided with a non-empty list, dates not in the list will be disabled in the calendar
  • Adds unit tests
  • Updates default story

Tickets?

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally

Security checklist?

  • No usage of dangerouslySetInnerHTML
  • Build output contains no secrets or API keys

Preview?

Screen.Recording.2026-04-14.at.3.20.01.PM.mov

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 14, 2026

🦋 Changeset detected

Latest commit: 12b8f01

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 14, 2026

Claude finished @vickiwyang's task in 1m 3s —— View job


Code Review

Nice, clean implementation! The logic in DatePicker.tsx:139-144 is correct — empty/undefined allowOnlyDatesList is a no-op, and the combination with futureDatesDisabled composes naturally.


♿ Accessibility

  • DateTableCell (DatePicker.tsx:157-174): disabled cells are missing aria-disabled={true}. The cell has role="gridcell" but screen readers won't know it's non-interactive without this attribute. This also affects the existing futureDatesDisabled path — worth fixing both in the same pass since you're already touching this area. Fix this →

💡 Suggestions (non-blocking)

  • DatePicker.stories.tsx:6: The default allowOnlyDatesList: [] in story args is functionally identical to undefined (the component guards with .length > 0). An empty array might confuse Storybook users expecting it to filter. Consider omitting it from args or adding it to argTypes with a proper control so it's explicitly configurable.
  • DateTableCell (Common.tsx): Disabled cells don't show cursor: not-allowed on hover — only the color changes. The trigger button already has this pattern (PopoverTrigger:17). Small UX win to add it for consistency.

  • Gather context and read changed files
  • Review implementation
  • Post review feedback

@vickiwyang vickiwyang requested a review from crisalbu April 14, 2026 19:23
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 14, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

1: Can you add a story to this please?
1: Would it be easier if we had a blacklist rather than a whitelist? It seems like the list of dates we want to allow would be very large compared to the list of dates we want to block

const isPresent = isSameDate(today, fullDate);
const isDisabled = futureDatesDisabled ? fullDate > today : false;
const isNotAllowed =
allowOnlyDatesList && allowOnlyDatesList.length > 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can be simplified to

Array.isArray(allowOnlyDateList) && !allowOnlyDatesList.some(d => isSameDate(d, fullDate))

Or even:

(allowOnlyDatesList && allowOnlyDatesList.length > 0) && !allowOnlyDatesList.some(d => isSameDate(d, fullDate))

Basically anytime we explicitly return a boolean value from an if or a ternary, it means it can be simplified into a single expression

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to the second suggested simplification. I wanted an empty array to work the same as not having an allowlist, and for the default Storybook control to be allowOnlyDatesList: [] rather than Set object (similar to predefinedDatesList on DateRangePicker):

Image

`;

interface CalendarProps {
allowOnlyDatesList?: Date[];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of the DatePickers, let's use Array<Date>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually better for simple types.

As mentioned in the previous discussion, the TypeScript team encourages T[]. For example, in the official documentation, compiler, and language server, you'll generally only find T[].

Apr-15-2026 10-35-07

Obs: In the demo, the TypeScript language server will show T[], even if the author writes Array<T]>.

We can still fall back to Array for declaring complex types, which we happen not to have at the moment, the ones you have there:

src/components/DatePicker/DatePicker.tsx:70:  const dayRefs = useRef<Array<HTMLTableCellElement | null>>([]);
src/components/DatePicker/Common.tsx:601:  const monthGridRef = useRef<Array<HTMLButtonElement | null>>([]);
src/components/DatePicker/Common.tsx:602:  const yearGridRef = useRef<Array<HTMLButtonElement | null>>([]);
src/components/DatePicker/Common.tsx:603:  const headerNavRefs = useRef<Array<HTMLButtonElement | null>>([null, null, null]);
src/components/DatePicker/Common.tsx:818:    const years: Array<number> = [];

If the author finds writing useRef<Array>([]) better than useRef<T[]>([]), that's completely fine.

In contrast, in the same component group, you find T[]:

src/components/DatePicker/utils.ts:62:): DateRange[] => {
src/components/DatePicker/utils.ts:66:    const lastSixMonths: DateRange[] = [];
src/components/DatePicker/utils.ts:81:  const nextSixMonths: DateRange[] = [];

That said, the shorthand is concise and more idiomatic, which is great too. I wouldn't bother going back and forth with comments on these:

src/components/DatePicker/Common.tsx:818:    const years: number[] = [];

This is exactly why linter rules exist. Enforcing one style over the other (which was proposed in a past PR) was already voted down.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@punkbit I've already changed this and the other place to Array<Date> following @hoorayimhelping's comments, but I'm happy to make any changes if needed.

Comment thread src/components/DatePicker/DatePicker.types.ts Outdated
@crisalbu
Copy link
Copy Markdown
Collaborator

@vickiwyang - in real life, for this scenario, how many dates would the users have that they can pick from?

Copy link
Copy Markdown
Contributor

@punkbit punkbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vickiwyang Thanks for your contribution! Let's introduce a Storybook story for this new state, please.

Comment thread .changeset/bil-5134-add-allowlist-param-to-datepicker.md
vickiwyang and others added 2 commits April 15, 2026 13:32
Co-authored-by: Helder Oliveira <helder.oliveira@clickhouse.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

Claude finished @vickiwyang's task in 1m 26s —— View job


Code Review

Nice implementation overall — the allowlist logic is clean and the new tests cover the important interaction cases well. A few things need attention before merge:


🚨 Must Fix

  • Changeset (bil-5134-add-allowlist-param-to-datepicker.md): The code block is unclosed — the file ends with /> inside a ```tsx fence that's never closed. This will produce malformed markdown in the published changelog. The closing ``` and the description line ("Only the dates in allowOnlyDatesList will be selectable…") from the punkbit suggestion are missing. Fix this →

  • Missing Storybook story: Two reviewers (hoorayimhelping, punkbit) explicitly requested a story demonstrating this feature. The current change only adds allowOnlyDatesList: [] to the default args, which is a no-op (empty array is guarded against). A new named export like WithAllowlist with a fixed set of dates would make the feature discoverable and testable in Storybook. Fix this →


♿ Accessibility

  • DateTableCell (DatePicker.tsx:157-174): Disabled cells are missing aria-disabled={true}. The cell has role="gridcell" but screen readers won't know it's non-interactive without this attribute. This also affects the pre-existing futureDatesDisabled path — worth fixing both in the same pass. Fix this →

💡 Suggestions (non-blocking)

  • isNotAllowed logic (DatePicker.tsx:139-142): As noted by hoorayimhelping, the if/else returning true/false can be collapsed to a single expression:

    const isNotAllowed =
      !!allowOnlyDatesList?.length && !allowOnlyDatesList.some(d => isSameDate(d, fullDate));
  • DateTableCell hover cursor: Disabled cells don't show cursor: not-allowed on hover — only the color changes. PopoverTrigger already uses this pattern (&:disabled { cursor: not-allowed }). Small UX win to add cursor: ${$isDisabled ? 'not-allowed' : 'pointer'} to the :hover block in Common.tsx.

  • allowOnlyDatesList: [] in default story args (DatePicker.stories.tsx:6): Functionally identical to undefined. Consider removing it from the default args to avoid confusing Storybook users, especially since a dedicated WithAllowlist story (above) would be the right place to demonstrate this.


  • Gather context and read changed files
  • Review implementation
  • Post review feedback

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

Claude finished @vickiwyang's task in 1m 26s —— View job


Code Review

The implementation is clean and the new tests cover the important cases well. A few things need attention before merge.


🚨 Must Fix

  • Changeset (bil-5134-add-allowlist-param-to-datepicker.md): The code block is still unclosed — the file ends on /> with no closing ``` and the description sentence is missing. This will produce malformed markdown in the published changelog. Add a closing ``` after />, then add: Only the dates in \allowOnlyDatesList` will be selectable; all other dates will be disabled.` Fix this →

  • Missing Storybook story: Two reviewers (hoorayimhelping, punkbit) have explicitly requested a story for this feature. The current change only adds allowOnlyDatesList: [] to the default args, which is a no-op (empty array is guarded by .length > 0). A dedicated named export like WithAllowlist with a fixed set of concrete dates is required per CONVENTIONS.md ("Storybook stories for all component states"). Also remove the no-op allowOnlyDatesList: [] from the default args to avoid confusing Storybook users. Fix this →


♿ Accessibility

  • DateTableCell (DatePicker.tsx:157-173): Disabled cells are missing aria-disabled. Per CONVENTIONS.md: "Disabled states need both disabled prop AND aria-disabled="true"". The cell has role="gridcell" but screen readers won't know it's non-interactive without this attribute. Use aria-disabled={isDisabled || undefined} so the attribute is omitted entirely when false (serializing false as "false" is technically valid but not useful). This also covers the pre-existing futureDatesDisabled path. Fix this →

💡 Suggestions (non-blocking)

  • isNotAllowed simplification (DatePicker.tsx:139-142): The if/else returning true/false can be collapsed to:
    const isNotAllowed = !!allowOnlyDatesList?.length && !allowOnlyDatesList.some(d => isSameDate(d, fullDate));
  • DateTableCell hover cursor: Disabled cells show no cursor: not-allowed on hover — only the color changes. PopoverTrigger already uses this pattern (&:disabled { cursor: not-allowed }). Adding cursor: ${({ $isDisabled }) => $isDisabled ? 'not-allowed' : 'pointer'} to the &:hover block in Common.tsx would be consistent.

  • Gather context and read changed files
  • Review implementation
  • Post review feedback

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

Claude finished @vickiwyang's task in 1m 17s —— View job


Code Review

Good progress since the last round — the isNotAllowed simplification landed cleanly and the DatePickerAllowOnlyNext30Days story addresses the reviewer requests. Two items still need attention before merge.


🚨 Must Fix

  • Changeset (bil-5134-add-allowlist-param-to-datepicker.md:17): The ```tsx code block opened on line 9 is never closed — the file ends at /> on line 17 with no closing ``` and no description sentence. This will produce malformed markdown in the published changelog. Add a closing ``` after />, then add: Only the dates in \allowOnlyDatesList` will be selectable; all other dates will be disabled.` Fix this →

  • allowOnlyDatesList: [] in default story args (DatePicker.stories.tsx:7): An empty array is functionally identical to undefined (the component guards with allowOnlyDatesList.length > 0). Leaving it in the default args implies the feature is active when it isn't, which will confuse Storybook users. Remove it from defaultStory.args — the dedicated DatePickerAllowOnlyNext30Days story is the right place to demonstrate this feature. Fix this →


♿ Accessibility

  • DateTableCell (DatePicker.tsx:157): Disabled cells are still missing aria-disabled. The cell has role="gridcell" but screen readers won't announce it as non-interactive without this attribute. Use aria-disabled={isDisabled || undefined} so the attribute is omitted when false (serializing false as "false" is valid but useless). Covers both futureDatesDisabled and allowOnlyDatesList paths. Fix this →

Already addressed from previous rounds ✅

  • isNotAllowed simplified to a single boolean expression
  • DatePickerAllowOnlyNext30Days story added with getNextNDatesForDatePickerAllowOnlyList(30)
  • Changeset updated with how-to section and description (just needs closing fence)
  • Tests cover allowlist-only, allowlist + futureDatesDisabled, and no-op empty array cases

  • Gather context and read changed files
  • Review implementation
  • Post review feedback

Copy link
Copy Markdown
Author

@vickiwyang vickiwyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoorayimhelping Added a story and implemented your suggestions.

@crisalbu @hoorayimhelping Grouping two related comments from the two of you:

in real life, for this scenario, how many dates would the users have that they can pick from?

Would it be easier if we had a blacklist rather than a whitelist? It seems like the list of dates we want to allow would be very large compared to the list of dates we want to block

For the bill date change use case, the users will have 30 days to pick from (starting from the day after tomorrow—April 17 given today's date of April 15—until but not including the same day on the subsequent month—May 16).

It's easier to define the whitelist than the blacklist for our use case, but maybe for other use cases, a blacklist would be easier. I also thought about defining the whitelist as a date range (using only start and end dates), but decided on an explicit array of dates for the flexibility of having non-contiguous dates. (@crisalbu For the bill date change use case, we could disable the organization's current bill date from the selection.)

`;

interface CalendarProps {
allowOnlyDatesList?: Date[];
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@punkbit I've already changed this and the other place to Array<Date> following @hoorayimhelping's comments, but I'm happy to make any changes if needed.

const isPresent = isSameDate(today, fullDate);
const isDisabled = futureDatesDisabled ? fullDate > today : false;
const isNotAllowed =
allowOnlyDatesList && allowOnlyDatesList.length > 0
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to the second suggested simplification. I wanted an empty array to work the same as not having an allowlist, and for the default Storybook control to be allowOnlyDatesList: [] rather than Set object (similar to predefinedDatesList on DateRangePicker):

Image

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

Claude finished @vickiwyang's task in 1m 6s —— View job


Code Review

Good progress this round — the changeset is now properly closed and the description sentence is in place. Two items remain before merge.


🚨 Must Fix

  • allowOnlyDatesList: [] in default story args (DatePicker.stories.tsx:7): An empty array is functionally identical to undefined (guarded by .length > 0), so this silently does nothing in the Default story. It will mislead Storybook users into thinking the feature is active. Remove it from defaultStory.args — the DatePickerAllowOnlyNext30Days story already demonstrates the feature correctly. Fix this →

♿ Accessibility

  • DateTableCell (DatePicker.tsx:157): Disabled cells are still missing aria-disabled. The cell has role="gridcell" but screen readers won't announce it as non-interactive without this attribute. Use aria-disabled={isDisabled || undefined} — the || undefined ensures the attribute is omitted entirely when false (serializing false as "false" is valid HTML but useless). This covers both the futureDatesDisabled and allowOnlyDatesList paths. Fix this →

Already fixed ✅

  • Changeset code block is now properly closed and the description sentence is present
  • isNotAllowed simplified to a single boolean expression
  • DatePickerAllowOnlyNext30Days story added with concrete dates
  • Tests cover allowlist-only, allowlist + futureDatesDisabled, and no-op empty array cases

  • Gather context and read changed files
  • Review implementation
  • Post review feedback

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

Claude finished @vickiwyang's task in 1m 11s —— View job


Code Review

Good progress since the last round — the changeset is now properly closed and the DatePickerAllowOnlyNext30Days story demonstrates the feature well. Two items remain before merge.


🚨 Must Fix

  • allowOnlyDatesList: [] in default story args (DatePicker.stories.tsx:7): An empty array is functionally identical to undefined (the component guards with allowOnlyDatesList.length > 0), so this silently does nothing in the Default story. Leaving it in args implies the feature is active when it isn't. Remove it — the DatePickerAllowOnlyNext30Days story already demonstrates the feature correctly. Fix this →

♿ Accessibility

  • DateTableCell (DatePicker.tsx:157): Disabled cells are still missing aria-disabled. The element has role="gridcell" but screen readers won't announce it as non-interactive without this attribute. Use aria-disabled={isDisabled || undefined} — the || undefined ensures the attribute is omitted entirely when false (serializing false as "false" is valid HTML but useless for AT users). This covers both futureDatesDisabled and allowOnlyDatesList paths. Fix this →

Already fixed ✅

  • Changeset code block is properly closed and the description sentence is present
  • isNotAllowed simplified to a single boolean expression
  • DatePickerAllowOnlyNext30Days story added with getNextNDatesForDatePickerAllowOnlyList(30)
  • Tests cover allowlist-only, allowlist + futureDatesDisabled, and no-op empty array cases

  • Gather context and read changed files
  • Review implementation
  • Post review feedback

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-qcx2650to-clickhouse.vercel.app

Built from commit: 077026fe42598ec10f001b09b4e76fd4524b8e0b

@vickiwyang
Copy link
Copy Markdown
Author

Screenshot 2026-04-15 at 4 17 52 PM

I do see this from Claude, but I believe that's how predefinedDatesList shows up as predefinedDatesList: [] in the default DateRangePicker controls (instead of Set object) as well? But I can remove if needed.

@punkbit
Copy link
Copy Markdown
Contributor

punkbit commented Apr 16, 2026

I do see this from Claude, but I believe that's how predefinedDatesList shows up as predefinedDatesList: [] in the default DateRangePicker controls (instead of Set object) as well? But I can remove if needed.

That's ok, the stories are changing during the migration process from styled components to CSS modules.

Copy link
Copy Markdown
Contributor

@punkbit punkbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vickiwyang Tested, seem to be working correctly. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants