feat: migrate NumericTextInput to Blend Design System#4641
feat: migrate NumericTextInput to Blend Design System#4641kanikabansal08 wants to merge 4 commits intomainfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@product-manager what do you think |
Let me read the main adapter file to understand the implementation fully. Solid, well-scoped migration. This follows the established adapter + feature flag pattern from the Accordion migration (#4405), which is the right incremental approach for the Blend rollout. What's good
Concerns to address
RecommendationApprove with minor changes:
The pattern is proven, the scope is tight, and the flag-gating makes this safe to ship. Good work by @kanikabansal08. |
|
@control_center_review_bot please review this pr |
| @@ -0,0 +1,85 @@ | |||
| let getFloat = strJson => strJson->JSON.Decode.string->Option.flatMap(val => val->Float.fromString) | |||
There was a problem hiding this comment.
🚨 Logic duplication detected — getFloat is defined identically in both NumericTextInput.res:1 and this adapter.
Remove the local definition and import from the existing module to maintain a single source of truth.
There was a problem hiding this comment.
where should I maintain this single source of truth @control_center_review_bot
There was a problem hiding this comment.
@control_center_review_bot where should I maintain this single source of truth
| let getFloat = strJson => strJson->JSON.Decode.string->Option.flatMap(val => val->Float.fromString) | ||
|
|
||
| @react.component | ||
| let make = ( |
There was a problem hiding this comment.
customStyleinputModeleftIcon/rightIconcustomPaddingClassleftIconCustomStyle/rightIconCustomStyle
These are all forwarded to NumericTextInput in the else branch (lines 69–83), but NumberInputBinding only receives value, onChange, disabled, placeholder, and maxLength.
Verify whether this is intentional — if Blend's NumberInput doesn't support these props, document that limitation. If it does, forward them consistently to avoid behavioral differences between branches.
| let blendValue = | ||
| input.value->JSON.Decode.float->Option.mapOr(Nullable.null, f => Nullable.make(f)) | ||
|
|
||
| let blendOnChange = ev => { |
There was a problem hiding this comment.
NumericTextInput.res:58–94 almost exactly — regex cleaning, decimal handling, precision slicing, and leading-zero removal.
Extract this to a shared utility or confirm why it must remain duplicated. If the logic diverges later, maintenance becomes risky.
| ~input: ReactFinalForm.fieldRenderPropsInput, | ||
| ~placeholder, | ||
| ~isDisabled=false, | ||
| ~inputMode="number", |
There was a problem hiding this comment.
🔍 inputMode is declared as a prop but never referenced in the Blend branch.
Since it's only passed to the legacy NumericTextInput, consider removing it from the adapter's signature if Blend's NumberInput doesn't support it, or pass it through if it does.
| @@ -0,0 +1,12 @@ | |||
| @react.component | |||
| let make = (~value: string, ~setValue: (string => string) => unit, ~hasError: bool=false) => { | |||
There was a problem hiding this comment.
🔍 OTPInputAdapter accepts hasError but the binding uses error (different prop name).
Confirm the Blend OTPInput component accepts error and that this mapping is intentional. If Blend uses a different prop naming convention, this could silently fail to display error states.
| @@ -8,7 +8,6 @@ open UserInfoTypes | |||
| * RouteUtils.getPath(~path="/payments", version) | |||
There was a problem hiding this comment.
Keep formatting-only changes in a separate PR to keep diffs focused and reviews efficient.
hs-control-center-bot
left a comment
There was a problem hiding this comment.
Verdict: 🔄 Request Changes
🚨 1 critical ·
The adapter pattern implementation is sound, but there's significant code duplication between the new adapter and the existing NumericTextInput component. The getFloat helper and the numeric parsing logic (regex cleaning, decimal handling, precision slicing) should be extracted to a shared utility. Additionally, several props are silently dropped in the Blend branch, which could cause behavioral inconsistencies between feature flag states.
|
❔ Can't tell what to update from this thread. The comment 'please update the skills' doesn't reference a specific finding or explain what rule needs changing. Reply to a specific finding you want changed with a short note on what's wrong, then mention me again with 'update the skill' or '/update'. |
|
@control_center_review_bot it instead of saying Remove the local definition and import from the existing module to maintain a single source of truth, we can say these type of things can go in logic utils. |
|
📝 Updated the reviewer based on this thread. Change: Softened the phrasing for logic duplication findings. Instead of prescriptive "Remove the local definition and import", the skill now suggests "consider whether this logic belongs in a shared utility" or importing if already in a utility file. Logical file: Applied to: runtime copy ( Runtime copy takes effect on the next review. Repo copy is uncommitted — a maintainer needs to land it so it survives the next redeploy. |
Type of Change
Description
Migrates
NumericTextInputto Blend Design System behind thedevBlendEnabledfeature flag.Adds
NumberInputBinding.res(raw Blend binding) andNumericTextInputAdapter.res(feature-flaggedadapter). Dead props (
~type_,~pattern,~autoComplete,~min,~max,~widthMatchwithPlaceholderLength,~iconOpacity,~shouldSubmitForm,~removeValidationCheck)are removed from the adapter signature — all live props are preserved.
Closes #4642
Motivation and Context
Part of the incremental Blend Design System migration. The adapter routes all 27 call sites
(4 direct + 23 via
InputFields.numericTextInputfactory) through Blend whendevBlendEnabled=true,with a clean legacy fallback. Precision and
removeLeadingZeroeslogic is preserved in the Blend branch.How did you test it?
Tested manually with
devBlendEnabledtoggled on and off across all 27 call sites:AmountFilter,AmountFilterUtils)Where to test it?
Checklist
npm run re:build