Apply BitTimePicker improvements (#12873) - #12877
Open
msynk wants to merge 5 commits into
Open
Conversation
WalkthroughBitTimePicker now supports seconds, constrained and stepped values, keyboard and pointer interaction, improved accessibility, callout lifecycle events, Now/Clear actions, responsive behavior, expanded styling hooks, comprehensive demos, and validation coverage. ChangesTimePicker improvements
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant BitTimePicker
participant TimePickerCallout
participant JSGestures
User->>BitTimePicker: Press key or pointer control
BitTimePicker->>TimePickerCallout: Open or update picker
TimePickerCallout->>BitTimePicker: Return time or action change
BitTimePicker->>JSGestures: Configure or release swipe handling
BitTimePicker-->>User: Render value and focus state
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cs (1)
628-638: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
ResetStyleBuildertoIsOpen. The source generator resetsStyleBuilderfor parameters with this attribute. Without it,Styles.Focusedcan remain stale whenIsOpenchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cs` around lines 628 - 638, Update the IsOpen parameter declaration in BitTimePicker to include the ResetStyleBuilder attribute, ensuring StyleBuilder is reset whenever IsOpen changes. Preserve the existing RegisterCssStyles implementation and style registrations.
🧹 Nitpick comments (3)
src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor (1)
421-421: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd SRI to both external icon stylesheets.
Add
crossorigin="anonymous"and theseintegrityvalues to all four links. Keep the rendered links and copyable sample identical.
- Font Awesome:
sha384-rWj9FmWWt3OMqd9vBkWRhFavvVUYalYqGPoMdL1brs/qvvqz88gvLShYa4hKNyqb- Bootstrap Icons:
sha384-XGjxtQfXaH2tnPFa9x+ruJTuLE3Aa6LhHSWRr1XeTyhezb4abCG4ccI5AkVDxqC+🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor` at line 421, Add SRI attributes to both external icon stylesheet links in BitTimePickerDemo.razor at lines 421-421 and the corresponding copyable sample in BitTimePickerDemo.razor.samples.cs at lines 256-278: add crossorigin="anonymous" and the specified Font Awesome or Bootstrap Icons integrity value to all four links, while keeping their rendered URLs and sample contents identical.src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.scss (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the empty
&.bit-rtlblock.The RTL close-button rule moved to
inset-inline-end, so this selector has no declarations left.♻️ Proposed cleanup
- - &.bit-rtl { - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.scss` around lines 60 - 61, Remove the empty &.bit-rtl selector block from the BitTimePicker styles, leaving the existing inset-inline-end RTL close-button rule unchanged.src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerTests.cs (1)
513-531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the press-and-hold test able to fail.
downTaskcompletes only after the continuous loop is cancelled.Task.WhenAny(downTask, Task.Delay(5000))therefore returns even when the loop never stops, and the assertionvalue >= new TimeSpan(11, 30, 0)is already satisfied by the single step thatHandleOnPointerDownperforms before the hold starts. A regression in the release path keeps this test green and adds a five-second wait.Await
downTaskdirectly with a timeout that fails the test, then assert that the value stops changing after the release.💚 Proposed test change
- var downTask = button.TriggerEventAsync("onpointerdown", new PointerEventArgs()); - - await button.TriggerEventAsync("onpointerup", new PointerEventArgs()); - - await Task.WhenAny(downTask, Task.Delay(5000)); - - Assert.IsTrue(value >= new TimeSpan(11, 30, 0)); + var downTask = button.TriggerEventAsync("onpointerdown", new PointerEventArgs()); + + await button.TriggerEventAsync("onpointerup", new PointerEventArgs()); + + var completed = await Task.WhenAny(downTask, Task.Delay(5000)); + Assert.AreSame(downTask, completed, "the press-and-hold loop did not stop on release"); + + var afterRelease = value; + Assert.IsTrue(afterRelease >= new TimeSpan(11, 30, 0)); + + await Task.Delay(300); + Assert.AreEqual(afterRelease, value, "the value kept changing after the release");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerTests.cs` around lines 513 - 531, Update BitTimePickerShouldStepWithAPointerPressAndStopOnRelease to await downTask with a timeout that fails the test if the pointer-down loop does not terminate, instead of using Task.WhenAny. After release and completion, verify the value no longer changes, ensuring the release path actually stops continuous stepping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cs`:
- Around line 628-638: Update the IsOpen parameter declaration in BitTimePicker
to include the ResetStyleBuilder attribute, ensuring StyleBuilder is reset
whenever IsOpen changes. Preserve the existing RegisterCssStyles implementation
and style registrations.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.scss`:
- Around line 60-61: Remove the empty &.bit-rtl selector block from the
BitTimePicker styles, leaving the existing inset-inline-end RTL close-button
rule unchanged.
In
`@src/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor`:
- Line 421: Add SRI attributes to both external icon stylesheet links in
BitTimePickerDemo.razor at lines 421-421 and the corresponding copyable sample
in BitTimePickerDemo.razor.samples.cs at lines 256-278: add
crossorigin="anonymous" and the specified Font Awesome or Bootstrap Icons
integrity value to all four links, while keeping their rendered URLs and sample
contents identical.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerTests.cs`:
- Around line 513-531: Update
BitTimePickerShouldStepWithAPointerPressAndStopOnRelease to await downTask with
a timeout that fails the test if the pointer-down loop does not terminate,
instead of using Task.WhenAny. After release and completion, verify the value no
longer changes, ensuring the release path actually stops continuous stepping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33d4c862-e9b5-48de-857a-1b86db749c41
📒 Files selected for processing (11)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.scsssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePickerClassStyles.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razorsrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor.samples.cssrc/BlazorUI/Demo/Client/Bit.BlazorUI.Demo.Client.Core/Pages/Components/Inputs/TimePicker/BitTimePickerDemo.razor.scsssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerTestModel.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/TimePicker/BitTimePickerValidationTest.razor
…into 12873-blazorui-timepicker-improvements
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…into 12873-blazorui-timepicker-improvements
…into 12873-blazorui-timepicker-improvements
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
1 participant
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.
closes #12873
Summary by CodeRabbit
Greptile Summary
The PR substantially expands BitTimePicker with seconds, time constraints, allowed-value predicates, action buttons, keyboard interaction, accessibility improvements, and additional customization.
Confidence Score: 4/5
The PR does not yet appear safe to merge because the previously reported event-handler wrapping defect remains outstanding.
The current TimePicker markup still directly binds the newly added interaction handlers, including paths that invoke consumer callbacks and predicates, so exceptions can bypass the project-required event-handling wrapper.
Files Needing Attention: src/BlazorUI/Bit.BlazorUI/Components/Inputs/TimePicker/BitTimePicker.razor
Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'develop' of https://github..." | Re-trigger Greptile