Skip to content

fix(time-field): keep scroll picker open while scrolling with closeOnSelect #812 - #826

Merged
ly-tempel-bitweb merged 3 commits into
rcfrom
fix/812-time-field-close-on-select-does-not-work-properly
Nov 4, 2026
Merged

ly-tempel-bitweb merged 3 commits into
rcfrom
fix/812-time-field-close-on-select-does-not-work-properly

Conversation

@ly-tempel-bitweb

@ly-tempel-bitweb ly-tempel-bitweb commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Closes #812

Summary by CodeRabbit

  • New Features
    • Time selection now distinguishes an explicit choice from scrolling or navigating the picker. With close-on-select enabled, the popover closes when a minute is selected or confirmed.
  • Bug Fixes
    • Selecting a minute before choosing an hour now keeps the hour shown on the picker.
    • Popovers now preserve focus on content that has already received focus.
  • Documentation
    • Clarified when the time picker popover closes, including behavior on mobile.

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
Walkthrough

The time picker now reports explicit selection commits, and TimeField uses those commits to control popover closing. The popover attachment logic now preserves focus when focus is already inside its container.

Changes: Time picker selection

Layer / File(s) Summary
Picker commit behavior
src/tedi/components/form/time-picker/time-picker.component.ts, src/tedi/components/form/time-picker/time-picker.component.spec.ts
The picker emits selectionCommitted for explicit minute or slot selections, but not for wheel navigation. Selecting a minute without a current value uses the parked hour. Scroll alignment clears programmatic-scroll state.
TimeField close-on-commit
src/tedi/components/form/time-field/time-field.component.ts, src/tedi/components/form/time-field/time-field.component.html, src/tedi/components/form/time-field/time-field.component.spec.ts, src/tedi/components/form/time-field/time-field.stories.ts
TimeField closes the popover on a committed selection when closeOnSelect is enabled. The documentation describes which scroll-picker actions commit a selection. invalidInput is now public readonly.

Popover attachment focus

Layer / File(s) Summary
Preserve focus inside popover
src/tedi/components/overlay/popover/popover.component.ts, src/tedi/components/overlay/popover/popover.component.spec.ts
The popover focuses its container on attachment only when the active element is outside it. Tests cover container focus and focus on an inner button.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant TimePickerComponent
  participant TimeFieldComponent
  participant PopoverComponent
  User->>TimePickerComponent: Select minute
  TimePickerComponent->>TimeFieldComponent: Emit selectionCommitted
  TimeFieldComponent->>PopoverComponent: Close when closeOnSelect is enabled
Loading

Possibly related PRs

  • TEDI-Design-System/angular#522: Adds a date-time field with scroll-based time controls, which is related to the scroll-picker behavior changed here.

Suggested reviewers: mart-sessman, intermetric

Merge Risk: Low · previous review ef92b

Keyboard users can close an empty time picker without selecting a time. This is a bounded issue with alternative selection methods.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Out of Scope Changes check Warning Most changes support #812, including picker tests and popover focus preservation. However, changing TimeFieldComponent.invalidInput from protected to public is an unrelated public API change. The … Remove the invalidInput visibility change, or provide a direct requirement that requires the input to be public.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: keeping the scroll picker open while scrolling when closeOnSelect is enabled. It is specific and related to the changeset.
Linked Issues check Passed The PR implements #812. TimePickerComponent emits selectionCommitted only for a minute click, Enter or Space in the minute column, and slot selections. Wheel scrolling, arrow navigation, and hour …
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check

Explanation

The PR implements #812. TimePickerComponent emits selectionCommitted only for a minute click, Enter or Space in the minute column, and slot selections. Wheel scrolling, arrow navigation, and hour selection do not emit it. TimeFieldComponent uses this event for closeOnSelect. Tests cover non-closing value changes and closing committed selections.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 7 files. (1 skipped: 1 unsupported.)

Full details: Out of Scope Changes check

Explanation

Most changes support #812, including picker tests and popover focus preservation. However, changing TimeFieldComponent.invalidInput from protected to public is an unrelated public API change. The linked issue does not require this visibility change.

Resolution

Remove the invalidInput visibility change, or provide a direct requirement that requires the input to be public.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between d86bf8c and ef92b79.

Files selected for processing: 8
  • src/tedi/components/form/time-field/time-field.component.html
  • src/tedi/components/form/time-field/time-field.component.spec.ts
  • src/tedi/components/form/time-field/time-field.component.ts
  • src/tedi/components/form/time-field/time-field.stories.ts
  • src/tedi/components/form/time-picker/time-picker.component.spec.ts
  • src/tedi/components/form/time-picker/time-picker.component.ts
  • src/tedi/components/overlay/popover/popover.component.spec.ts
  • src/tedi/components/overlay/popover/popover.component.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@ly-tempel-bitweb

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov

codecov Bot commented Nov 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • Fix comments: Fix CodeRabbit comments on this PR

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

♿ Accessibility — ✅ no blocking violations

No accessibility violations in the components changed by this PR.

🔕 Known issues — 20 stories marked todo (warn only)

Stories

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

🎨 Chromatic

No visual changes.

Built from 0c93c64. Commits pushed after this are not covered; approve again to rebuild.

View the build

@ly-tempel-bitweb
ly-tempel-bitweb merged commit 48efc6f into rc Nov 4, 2026
40 of 42 checks passed
@ly-tempel-bitweb
ly-tempel-bitweb deleted the fix/812-time-field-close-on-select-does-not-work-properly branch November 4, 2026 08:53
github-actions Bot pushed a commit that referenced this pull request Nov 4, 2026
## [9.0.1-rc.2](angular-9.0.1-rc.1...angular-9.0.1-rc.2) (2026-11-04)

### Bug Fixes

* **time-field:** keep scroll picker open while scrolling with closeOnSelect [#812](#812) ([#826](#826)) ([48efc6f](48efc6f))
* **timeline:** collapse empty title, description and content slots [#735](#735) ([#831](#831)) ([6e7ad24](6e7ad24))

This branch was successfully deployed

1 active deployment
github-pages — 0c93c64b Deployed Nov 3, 2026 by ly-tempel-bitweb via Deploy #1371
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.

[TimeField]: closeOnSelect doesnt work properly

3 participants