Skip to content

Conversation

@Greexter
Copy link
Contributor

@Greexter Greexter commented Dec 26, 2025

closes #11926

Summary by CodeRabbit

  • Refactor
    • Improved NumberField component's input value synchronization mechanism to use two-way binding, enhancing how the component handles value updates between immediate and change modes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 26, 2025

Walkthrough

The BitNumberField component's input binding is refactored to use Blazor's :get and :set binding modifiers instead of explicit event handlers. A new handler method is introduced to route value changes through the appropriate async method based on the Immediate flag, ensuring consistent display of corrected values when constraints are applied.

Changes

Cohort / File(s) Summary
Input binding pattern
src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor
Replaces explicit onchange/oninput event handlers with @bind-value:get, @bind-value:set, and @bind-value:event modifiers; removes manual value attribute binding in favor of the get-binding pattern; retains focus, blur, wheel, and other event handlers
Value set delegation
src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs
Adds new private async method HandleOnStringValueSet(string? value) that constructs a ChangeEventArgs and delegates to either HandleOnStringValueInputAsync or HandleOnStringValueChangeAsync based on the Immediate flag

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A field of numbers, now so true,
Bind-value modifiers made brand new,
No more dancing 'tween old and corrected,
The value displayed is now protected—
Constraints respected, consistent and bright!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Use get/set modifiers for BitNumberField value binding' clearly and concisely describes the main technical change, which aligns with the changeset's primary modifications.
Linked Issues check ✅ Passed The PR successfully implements the fix for issue #11926 by introducing @bind-value:get and @bind-value:set modifiers with event switching based on Immediate flag, addressing the root cause of value display inconsistency.
Out of Scope Changes check ✅ Passed All changes are scoped to BitNumberField's value binding implementation to resolve the display inconsistency issue; no unrelated modifications were introduced.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 74be421 and 1095937.

📒 Files selected for processing (2)
  • src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor
  • src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/BitInputBase.cs (3)
  • Task (138-236)
  • Task (320-374)
  • Task (376-390)
🔇 Additional comments (3)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs (1)

372-379: LGTM - Correct implementation of the :set binding handler.

The new HandleOnStringValueSet method correctly routes incoming string values through the appropriate handler based on the Immediate flag. This enables the :get/:set binding modifiers to work properly, ensuring that after value clamping/normalization, the displayed input reflects the corrected CurrentValueAsString.

src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor (2)

1-1: Minor cleanup - LGTM.

Namespace declaration cleanup (likely removed a BOM or extraneous character).


114-116: The binding implementation uses correct :get/:set/:event modifiers.

The code at lines 114-116 correctly implements the pattern where CurrentValueAsString getter retrieves the formatted current value, HandleOnStringValueSet processes the input, and the binding event switches between oninput and onchange based on the Immediate flag. This should ensure the displayed value stays in sync with the component's processed value.

However, complete verification requires runtime testing across:

  1. Repeated out-of-bounds entries with Min/Max constraints
  2. Precision rounding with decimal types
  3. Both Immediate=true (oninput) and Immediate=false (onchange) modes
  4. Various numeric types (int, decimal, double, float)

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@Greexter
Copy link
Contributor Author

I only changed the BitNumberField component but I am not sure if similar changes should be made to other input components such as BitTextField since :get and :set are not used there either. I am not aware of any concrete issues in those other components tho, only that using the modifiers is recommended for 2 way binding with input.

@msynk
Copy link
Member

msynk commented Dec 27, 2025

@Greexter Thanks for your contribution.
About applying the same changes to other input components:
Actually, I'm not happy with using Blazor binding for the input element, since it utilizes too much of Blazor internals at the low level of our components. If there were another solution, I would like to apply it. But unfortunately, this is the only solution to render the actual value while the value is getting reset to the same unchanged number.
So for other inputs, as long as there is no need to use this change, I'm ok with the current implementations of them.

@msynk
Copy link
Member

msynk commented Dec 28, 2025

I'm going to merge this PR and address the review comments myself. thanks :)

@msynk msynk merged commit beb167a into bitfoundation:develop Dec 28, 2025
3 checks passed
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.

BitNumberField displayed value may be inconsistent with the actual current value

2 participants