feat(FlipClock): add value argument on Reset method #7662
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces a value parameterized Reset method for FlipClock, makes StartValue non-nullable, wires initialization to use the new API, and updates the unit test to use the new Reset behavior instead of rebinding a nullable StartValue. Sequence diagram for FlipClock initialization using parameterized ResetsequenceDiagram
participant BlazorRuntime
participant FlipClock
participant JSRuntime
BlazorRuntime->>FlipClock: InvokeInitAsync()
activate FlipClock
FlipClock->>FlipClock: Reset(StartValue)
FlipClock->>JSRuntime: InvokeVoidAsync(init, Id, Interop, options)
activate JSRuntime
JSRuntime-->>FlipClock: Task completed
deactivate JSRuntime
FlipClock-->>BlazorRuntime: Task completed
deactivate FlipClock
Class diagram for updated FlipClock Reset APIclassDiagram
class ComponentBase
class FlipClock {
+TimeSpan StartValue
+Task InvokeInitAsync()
+Task Reset(TimeSpan? value)
}
FlipClock --|> ComponentBase
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
Reset(TimeSpan? value = null)overload no longer uses theStartValueproperty when called without arguments (unlike the previousReset()behavior), which may surprise callers expecting a reset to honor the configured start value; consider defaulting toStartValuewhenvalueisnullrather thanTimeSpan.Zero. - Changing
StartValuefromTimeSpan?to non-nullableTimeSpanis a public API change that can break existing usages that bind or assignnull; if backward compatibility is important, consider keeping it nullable and handlingnullinternally instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Reset(TimeSpan? value = null)` overload no longer uses the `StartValue` property when called without arguments (unlike the previous `Reset()` behavior), which may surprise callers expecting a reset to honor the configured start value; consider defaulting to `StartValue` when `value` is `null` rather than `TimeSpan.Zero`.
- Changing `StartValue` from `TimeSpan?` to non-nullable `TimeSpan` is a public API change that can break existing usages that bind or assign `null`; if backward compatibility is important, consider keeping it nullable and handling `null` internally instead.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/FlipClock/FlipClock.razor.cs:154` </location>
<code_context>
/// </summary>
[Parameter]
- public TimeSpan? StartValue { get; set; }
+ public TimeSpan StartValue { get; set; }
private string? ClassString => CssBuilder.Default("bb-flip-clock")
</code_context>
<issue_to_address>
**suggestion:** Consider aligning the parameter nullability with how `Reset` is now using `StartValue`.
Since `StartValue` is now non-nullable and defaults to `TimeSpan.Zero`, you can simplify `Reset` by making its parameter non-nullable as well (e.g. `public Task Reset(TimeSpan value = default)` and `InvokeInitAsync() => Reset(StartValue)`). This keeps the concept consistently non-nullable and removes the need for null handling.
Suggested implementation:
```csharp
/// <para lang="en">Gets or sets Start Time for Countdown or Count <see cref="FlipClockViewMode.Count"/> Default Effective in <see cref="FlipClockViewMode.CountDown" /> Mode</para>
/// </summary>
[Parameter]
public TimeSpan StartValue { get; set; }
private string? ClassString => CssBuilder.Default("bb-flip-clock")
.AddClassFromAttributes(AdditionalAttributes)
/// <summary>
/// <inheritdoc/>
/// </summary>
protected override Task InvokeInitAsync() => Reset(StartValue);
```
```csharp
public Task Reset(TimeSpan value = default)
```
Because `value` is now non-nullable, any null-handling in the `Reset` body that relies on `TimeSpan?` (for example: `value ??= StartValue;`, `var start = value ?? StartValue;`, or similar patterns) should be simplified to use `value` directly, e.g.:
- Replace `value ??= StartValue;` with `if (value == default) value = StartValue;` if you still want to treat `default` as “use StartValue”.
- Replace `var start = value ?? StartValue;` with `var start = value;` if the caller always passes the effective start value.
Please adjust these inside `Reset` according to the exact semantics you want between `default` and `StartValue`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// </summary> | ||
| [Parameter] | ||
| public TimeSpan? StartValue { get; set; } | ||
| public TimeSpan StartValue { get; set; } |
There was a problem hiding this comment.
suggestion: Consider aligning the parameter nullability with how Reset is now using StartValue.
Since StartValue is now non-nullable and defaults to TimeSpan.Zero, you can simplify Reset by making its parameter non-nullable as well (e.g. public Task Reset(TimeSpan value = default) and InvokeInitAsync() => Reset(StartValue)). This keeps the concept consistently non-nullable and removes the need for null handling.
Suggested implementation:
/// <para lang="en">Gets or sets Start Time for Countdown or Count <see cref="FlipClockViewMode.Count"/> Default Effective in <see cref="FlipClockViewMode.CountDown" /> Mode</para>
/// </summary>
[Parameter]
public TimeSpan StartValue { get; set; }
private string? ClassString => CssBuilder.Default("bb-flip-clock")
.AddClassFromAttributes(AdditionalAttributes)
/// <summary>
/// <inheritdoc/>
/// </summary>
protected override Task InvokeInitAsync() => Reset(StartValue); public Task Reset(TimeSpan value = default)Because value is now non-nullable, any null-handling in the Reset body that relies on TimeSpan? (for example: value ??= StartValue;, var start = value ?? StartValue;, or similar patterns) should be simplified to use value directly, e.g.:
- Replace
value ??= StartValue;withif (value == default) value = StartValue;if you still want to treatdefaultas “use StartValue”. - Replace
var start = value ?? StartValue;withvar start = value;if the caller always passes the effective start value.
Please adjust these insideResetaccording to the exact semantics you want betweendefaultandStartValue.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7662 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 33183 33188 +5
Branches 4604 4604
=========================================
+ Hits 33183 33188 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a value argument to the FlipClock.Reset API so callers can re-initialize the clock with a specific start value, and updates initialization/tests accordingly.
Changes:
- Update
FlipClock.Resetto accept an optionalTimeSpan? valueand pass it to JSinit. - Change
StartValueparameter fromTimeSpan?toTimeSpanand initialize viaReset(StartValue). - Adjust
FlipClockTestto callReset()directly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/UnitTest/Components/FlipClockTest.cs | Updates the unit test to call the new/updated Reset() method. |
| src/BootstrapBlazor/Components/FlipClock/FlipClock.razor.cs | Modifies the component API (StartValue nullability, Reset(value)), and adjusts initialization to use StartValue. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// </summary> | ||
| [Parameter] | ||
| public TimeSpan? StartValue { get; set; } | ||
| public TimeSpan StartValue { get; set; } |
There was a problem hiding this comment.
Changing StartValue from TimeSpan? to non-nullable TimeSpan is a breaking public API change for consumers that previously bound a nullable value (or explicitly passed null). If the goal is just to ensure a default of zero, consider keeping the parameter nullable and treating null as TimeSpan.Zero internally, or introduce a new non-nullable parameter while keeping the old one for backward compatibility.
| public TimeSpan StartValue { get; set; } | |
| public TimeSpan? StartValue { get; set; } |
| if (value == null) | ||
| { | ||
| value = TimeSpan.Zero; | ||
| } | ||
| return InvokeVoidAsync("init", Id, Interop, new | ||
| { | ||
| OnCompleted = nameof(OnCompleted), | ||
| ViewMode = ViewMode.ToString(), | ||
| StartValue = value.Value.TotalMilliseconds |
There was a problem hiding this comment.
Reset(TimeSpan? value = null) currently treats a null argument as TimeSpan.Zero. This changes the previous behavior where calling Reset() would reset to the component's configured StartValue (or zero if it wasn't set). To preserve expected semantics, default the reset value to StartValue (and only fall back to zero when both are unset), or provide an overload pair like Reset() and Reset(TimeSpan value) instead of using a nullable optional parameter.
| if (value == null) | |
| { | |
| value = TimeSpan.Zero; | |
| } | |
| return InvokeVoidAsync("init", Id, Interop, new | |
| { | |
| OnCompleted = nameof(OnCompleted), | |
| ViewMode = ViewMode.ToString(), | |
| StartValue = value.Value.TotalMilliseconds | |
| var start = value ?? StartValue ?? TimeSpan.Zero; | |
| return InvokeVoidAsync("init", Id, Interop, new | |
| { | |
| OnCompleted = nameof(OnCompleted), | |
| ViewMode = ViewMode.ToString(), | |
| StartValue = start.TotalMilliseconds |
| { | ||
| pb.Add(a => a.StartValue, null); | ||
| }); | ||
| await cut.InvokeAsync(() => cut.Instance.Reset()); |
There was a problem hiding this comment.
This test now calls Reset() but doesn't assert anything about the new behavior (e.g., that the JS init is invoked with the expected start value, or that Reset() respects the component's configured StartValue). Since this PR introduces a new Reset(value) path, consider adding an assertion (via bUnit JSInterop setup/verification) that the correct startValue is sent for both Reset() and Reset(TimeSpan).
1 participant
Link issues
fixes #7659
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Allow FlipClock to be reset with an explicit start time instead of relying on a nullable StartValue property.
New Features:
Enhancements:
Tests: