feat(time-picker): apply relative options on click - #682
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Picking from the dropdown list required a second press on Apply. That confirmation earns its keep in the calendar, where a half-picked range is not yet a valid selection, but it is pure friction in the option list where the click is itself the decision. Apply and Cancel now render only while a calendar is on screen — `showCalendar` on, or "Custom Interval" picked. In the plain option list a click commits and closes, matching `kv-single-select-dropdown` and every other dropdown here. The footer is dropped entirely when it would hold neither the toggle nor the actions, instead of leaving an empty bordered bar. The commit is driven by a new `relativeTimeOptionClicked` event rather than `selectedRelativeTimeChange`, which `kv-relative-time-picker` also re-emits from its 10s refresh as a "now"-relative range moves — committing from that would close an open panel roughly once a minute. It mirrors the existing `customizeIntervalClicked` intent event, and fires even when the clicked option is already selected so a re-click still confirms. A timezone change in the option list commits without closing, since no Apply is left to confirm it, and re-anchors the range to the new timezone. Hiding the calendar now also discards a pending range, which would otherwise be stranded with no way to apply it. No API change: `timeRangeChange` keeps its name and `ITimePickerTime` payload, and still fires once per selection — only earlier, on the click instead of on Apply. Consumers need no code change. What does change downstream is anything driving the old two-step flow: automation or e2e tests that press Apply in the option list, and `cancelClicked`, which is no longer reachable from that view (dismissal still reports `dropdownStateChange(false)`).
There was a problem hiding this comment.
🟡 Changes recommended
A newly added calendar-mode unit test doesn’t model real click behavior (missing the paired selectedRelativeTimeChange), so it asserts an incorrect state and could mask regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates kv-time-picker and kv-relative-time-picker so that selecting a relative time option commits immediately on click (and closes the dropdown), while Apply/Cancel are only shown when a calendar-based view is active, reducing friction in the simple option-list flow.
Changes:
kv-time-picker: commit-and-close on relative option click (when no calendar is visible) and commit-without-close on timezone changes in the option list.kv-time-picker: render footer actions (Apply/Cancel) only for calendar-visible views; drop the footer entirely when it would be empty.kv-relative-time-picker: introduce a newrelativeTimeOptionClickedintent event to distinguish real user clicks from the periodic “now-relative” refresh emissions.
File summaries
| File | Description |
|---|---|
| packages/ui-components/src/components/time-picker/time-picker.tsx | Adds click-to-commit behavior for relative options, restructures commit vs publish logic, and conditionally renders footer/actions based on view. |
| packages/ui-components/src/components/time-picker/test/time-picker.spec.tsx | Adds unit coverage for click-to-commit, timezone commit behavior, calendar-only Apply/Cancel visibility, and calendar-hide rollback behavior. |
| packages/ui-components/src/components/relative-time-picker/relative-time-picker.types.ts | Exposes the new relativeTimeOptionClicked event in the public component event interface. |
| packages/ui-components/src/components/relative-time-picker/relative-time-picker.tsx | Emits relativeTimeOptionClicked on option selection (including re-clicking the already-selected option). |
| packages/ui-components/src/components/relative-time-picker/readme.md | Regenerates docs to include the new event in the Events table. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
djcouto
force-pushed
the
dc/time-picker
branch
from
September 8, 2026 15:15
3ff041f to
08744c4
Compare
djcouto
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ff041f8bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Committing a timezone change in the option list assumed a selection was
already in place. When the given `relativeTimePickerOptions` omit the default
key nothing is preselected — `selectedTimeState` starts as `{ key: '', range: [] }`
— so touching the timezone first emitted a `timeRangeChange` whose empty range
breaks `ITimePickerTime` and hands a consumer an unusable range. The cast in
`getTimePickerEventPayload` kept `tsc` quiet about it.
The commit now needs a complete range as well as a calendar-free view, reusing
the `validateNewRange` check behind Apply. Otherwise the timezone is kept as
draft, so the next option click still carries it.
Also fixes the spec helper, which dispatched only `relativeTimeOptionClicked`.
A real click on a different option emits `selectedRelativeTimeChange` too, so
the calendar-mode test asserted a draft state that cannot happen and would have
masked a regression in it; the helper now models both events, and takes
`changed: false` for a re-click on the selected option.
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is implemented with clear separation between “draft updates” and “user intent commits,” and it’s backed by focused regression tests covering the key interaction modes.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
ricardogoncalves89
approved these changes
Sep 10, 2026
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
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
kv-time-pickerrequired a second press on Apply to commit a selection picked from the dropdown list. That confirmation earns its keep in the calendar, where a half-picked range is not yet a valid selection, but it is pure friction in the plain option list where the click is itself the decision.Apply and Cancel now render only while a calendar is on screen —
showCalendaron, or Custom Interval picked. In the option list a click commits and closes, matchingkv-single-select-dropdownand every other dropdown in the library.Note
Behaviour change, not an API change.
timeRangeChangekeeps its name and itsITimePickerTimepayload, and still fires once per selection — only earlier, on the click instead of on Apply. Consumers need no code change, so this is a minor, not a major.What does change downstream is anything driving the old two-step flow: automation or e2e tests that press Apply in the option list will no longer find the button, and
cancelClickedis no longer reachable from that view (dismissal still reportsdropdownStateChange(false)). Nothing in this repo consumeskv-time-pickerinternally.Changes
feat(time-picker)— Apply/Cancel visibility is derived from the view rather than always rendered:isCalendarVisible()istimePickerView !== RelativeTimePicker, which covers exactly the two calendar cases (showCalendar→FullView, Custom Interval →AbsoluteTimePicker). No new prop.feat(time-picker)—onClickApplyis split intoemitTimeRangeChange(publish) andcommitTimeState(publish + dismiss), so the new click path reuses the existing Apply behaviour instead of duplicating it. The Apply payload is unchanged.feat(relative-time-picker)— newrelativeTimeOptionClickedevent, emitted only from the click handler. This is the crux:selectedRelativeTimeChangeis also re-emitted from the component's 10-secondsetIntervalas a "now"-relative range moves (hasRangeChangedcompares with seconds zeroed), so committing from it would have made an open panel commit and close itself roughly once a minute. The new event mirrors the existingcustomizeIntervalClickedintent-event pattern, and fires even when the clicked option is already selected so a re-click still confirms.feat(time-picker)— a timezone change in the option list commits without closing the panel, since no Apply is left to confirm it. The range is re-anchored to the new timezone by reusinggetRelativeTimeOption+buildOptionRange/buildTimestampRange, so a boundary-based option like Today shifts correctly.fix(time-picker)— hiding the calendar now discards a pending range viaundoLastChanges, which would otherwise be stranded with no Apply to commit it and would leavecalendarViewLockedset, disabling the toggle.fix(time-picker)— the footer is dropped entirely when it would hold neither the toggle nor the actions (displayCalendarToggle={false}in the option list), instead of leaving an empty bordered, padded bar.test(time-picker)— 14 new specs, including a regression guard asserting thatonSelectedRelativeTimeChangealone (the periodic-refresh path) emits nothing and leaves the panel open.packages/ui-components/src/components/relative-time-picker/readme.mdis regenerated by the build. No enum was added, sopackages/react-ui-components/src/ui-components.tsneeds no change, and the React binding picks the event up through the generated JSX types.Test Plan
pnpm build:packages— exit 0, no errorspnpm test— 421 specs pass, 123 snapshots passpnpm lint— cleanManual steps —
pnpm storybook, then Time Picker → Time Picker, watching the Actions panel:timeRangeChange(key: last-7-d), thendropdownStateChange(false), panel closed, input reads "Last 7 days". Re-open and click the already selected option — it still commits and closes.timeRangeChangefires and the panel stays open. Switching between two zones on Today shows the range re-anchor (e.g.Pacific/Honolulu→2026-09-08T10:00Z,Europe/Lisbon→2026-09-07T23:00Z).displayCalendarToggle: false,displayTimezoneDropdown: false) — no footer bar at all; clicking a duration commits and closes.