Skip to content

chore(DateTimeRangePicker): rename to DateTimePicker to DateTimeRangePicker#882

Open
hoorayimhelping wants to merge 2 commits intomainfrom
bucky-rename-datetimepicker
Open

chore(DateTimeRangePicker): rename to DateTimePicker to DateTimeRangePicker#882
hoorayimhelping wants to merge 2 commits intomainfrom
bucky-rename-datetimepicker

Conversation

@hoorayimhelping
Copy link
Member

@hoorayimhelping hoorayimhelping commented Mar 6, 2026

It does what it says on the tin

@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2026

🦋 Changeset detected

Latest commit: 6f59ee0

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

@hoorayimhelping hoorayimhelping requested a review from ariser March 6, 2026 20:59
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @hoorayimhelping's task in 1m 15s —— View job


Code Review

  • Gather context
  • Review changed files
  • Post findings

This PR renames the DateTimePicker component to DateTimeRangePicker for naming clarity. The changes are straightforward and consistent across the component file, tests, and stories.

💡 Suggestions (non-blocking)

  • utils.ts: The helper function getPredefinedTimePeriodsForDateTimePicker (line 93) still uses the old DateTimePicker name. For naming consistency, consider renaming it to getPredefinedTimePeriodsForDateTimeRangePicker. Since neither the component nor this function is currently exported from src/index.ts, this isn't a breaking change — but it's worth keeping names consistent within the module.

  • src/index.ts: Neither the old DateTimePicker nor the new DateTimeRangePicker is exported from the package's public API. If this component is intended for consumers, it should be added to src/index.ts (along with DateTimeRangePickerProps and getPredefinedTimePeriodsForDateTimeRangePicker). This is pre-existing and not introduced by this PR, but a good opportunity to address.

Otherwise, the rename is clean and complete — all usages in the component, tests, stories, and internal imports are updated correctly. ✅

@hoorayimhelping hoorayimhelping requested a review from punkbit March 6, 2026 21:00
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @hoorayimhelping's task in 1m 29s —— View job


Code Review

  • Gather context
  • Review changed files
  • Post findings

Clean rename — the mechanics are correct throughout. One thing to flag:

🚨 Must Fix

  • Missing public export: DateTimeRangePicker (like its predecessor DateTimePicker) is not exported from src/index.ts or src/components/DatePicker/index.ts. Consumers can't import the component without a direct file path import. If this is intentional (i.e., it'll be added in a follow-up), that's fine — but the changeset is marked minor, which implies a user-visible public API change. Either:

    • Add the export to src/index.ts:
      export { DateTimeRangePicker } from './components/DatePicker/DateTimeRangePicker';
      export type { DateTimeRangePickerProps } from './components/DatePicker/DateTimeRangePicker';
    • Or downgrade the changeset to patch if this is a purely internal rename with no public API impact yet.

    Fix this →

💡 Suggestions (non-blocking)

  • data-testid consistency: The input element in Common.tsx:291 still uses data-testid="datetimepicker-input". The tests reference the same ID so it's self-consistent — but it's worth updating to "datetimerangepicker-input" to match the new component name.

Everything else looks great — files correctly renamed, all references updated, stories and tests properly migrated. 🙌

@hoorayimhelping hoorayimhelping force-pushed the bucky-rename-datetimepicker branch from 34a3d88 to 6f59ee0 Compare March 6, 2026 21:08
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @hoorayimhelping's task in 57s —— View job


Code Review

  • Gather context
  • Review changed files
  • Post findings

Clean rename — all references in the component, tests, stories, and internal imports are correctly updated. One thing still needs attention:

🚨 Must Fix

  • Changeset/export mismatch: The changeset (wild-pillows-agree.md) is marked minor and states "DateTimePicker has been renamed to DateTimeRangePicker" — implying a public API change. But DateTimeRangePicker is still not exported from src/index.ts (the package root), only from src/components/DatePicker/index.ts. Since neither the old DateTimePicker nor the new DateTimeRangePicker was ever accessible via the package's public API, this rename has no public API impact yet. Either:
    • Add the export to src/index.ts to make it public (consistent with DatePicker and DateRangePicker):
      export { DateTimeRangePicker } from './components/DatePicker/DateTimeRangePicker';
      export type { DateTimeRangePickerProps } from './components/DatePicker/DateTimeRangePicker';
    • Or downgrade the changeset from minor to patch if this is still an internal component. Fix this →

💡 Suggestions (non-blocking)

  • utils.ts:93: getPredefinedTimePeriodsForDateTimePicker still uses the old DateTimePicker name. Consider renaming to getPredefinedTimePeriodsForDateTimeRangePicker for consistency within the module.

  • Common.tsx:291: data-testid="datetimepicker-input" still references the old name. The tests use the same ID so it's self-consistent, but updating to "datetimerangepicker-input" would match the new component name.

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

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

Built from commit: bc34525bb3eb3d04f456b856ffb825ce558308e2

Copy link
Contributor

@DreaminDani DreaminDani left a comment

Choose a reason for hiding this comment

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

Is this a breaking change? Do we have "notify" downstream users (control-plane)?

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.

2 participants