You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bug fix (non-breaking change which fixes an issue)
Description
When decimalPlaces is configured on the Slider widget, displayed values were silently stripping trailing zeros:
Mark labels: 10.00 rendered as 10, 9.20 rendered as 9.2
Tooltip: same raw number passed through with no formatting applied
Root causes:
marks.ts — parseFloat(value.toFixed(n)).toString() round-tripped through a number, discarding trailing zeros. Fixed by keeping the toFixed string directly as the label and using parseFloat only for the numeric key.
createHandleRender.tsx — tooltip overlay received restProps.value (raw RC Slider number). Fixed by adding a decimalPlaces parameter and applying .toFixed(decimalPlaces) before passing to the overlay.
Files changed:
src/utils/marks.ts — fix label generation
src/utils/createHandleRender.tsx — accept and apply decimalPlaces
src/components/Container.tsx — pass decimalPlaces to createHandleRender
src/utils/__tests__/marks.spec.ts — new unit tests
src/utils/__tests__/createHandleRender.spec.tsx — new unit tests
What should be covered while testing?
Configure a Slider widget with decimalPlaces = 2 and Number of markers > 0.
Set min/max so some markers land on whole numbers (e.g. min=0, max=10, markers=2).
Mark labels should show 0.00, 5.00, 10.00 — not 0, 5, 10.
Drag the handle to a whole number value — tooltip should show 10.00, not 10.
Drag to a value with one decimal (e.g. 9.5) — tooltip should show 9.50.
Set decimalPlaces = 0 and verify no change in existing behavior (labels and tooltip show integers without decimal point).
New unit tests: tooltip formatting across decimalPlaces values
Skipped (out of scope): dist/, pnpm-lock.yaml
CI checks: could not be fetched in this environment — please verify they are green before merging.
Findings
⚠️ Low — parseFloat(label) breaks mark keys when locale uses comma separator
File:packages/pluggableWidgets/slider-web/src/utils/marks.ts line 29 Note: When a non-. decimal separator is active (e.g. ,), formatDecimal returns "5,00". parseFloat("5,00") returns 5 (truncated), which is coincidentally correct for whole numbers but will be wrong for fractional values — parseFloat("4,60") returns 4, not 4.6. This would cause mark positions to be misplaced on the slider track.
Fix: Compute the numeric key from the raw unformatted number rather than parsing the formatted string:
The marks.spec.ts locale test currently only checks string label values (marks![0], marks![5], marks![10]) — all happen to be whole numbers, so the key truncation is not caught. A test with min: 0, max: 9.2 and a comma locale would expose the bug.
⚠️ Low — useMemo dep array omits sliderRef
File:packages/pluggableWidgets/slider-web/src/components/Container.tsx line 44 Note:sliderRef is passed into createHandleRender but is absent from the useMemo dependency array. sliderRef is a useRef value and its .current is intentionally mutable, so it doesn't need to be in the dep array by React convention. This is technically correct — just worth a brief comment so the next reader doesn't add it unnecessarily. No code change required.
Positives
Root-cause analysis in the PR description is accurate and clearly written — the two distinct bugs (mark label and tooltip) are explained separately with their specific fixes.
Extracting formatDecimal into helpers.ts as a shared utility is the right call; both marks.ts and createHandleRender.tsx now use a single code path.
Wrapping createHandleRender in useMemo is a nice bonus improvement — avoids recreating the render function on every render.
global.d.ts typing the window.mx locale chain with all-optional fields is correct defensive typing; the tests reset window.mx in afterEach which prevents test pollution.
CHANGELOG entry is present and follows Keep a Changelog format.
New unit tests for mark label formatting including edge cases
Skipped (out of scope): dist/, pnpm-lock.yaml
CI status at review time: triage, Check SHA in GH Actions, Snyk (code/license/security) all passed; Run code quality check, E2E matrix, and sigridci still in progress.
Findings
⚠️ Low — decimalPlaces param in createHandleRender is redundant with formatter
File:packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx line 31 Note: The branch decimalPlaces > 0 ? formatter.format(...) : String(...) uses decimalPlaces only as a guard. But when decimalPlaces === 0, calling formatter.format(new Big(value)) would produce the same result ("10", no decimal point) because the formatter was already configured with decimalPrecision: 0. The guard and the extra decimalPlaces parameter in CreateHandleRenderProps can be simplified:
// beforeconstformattedValue=decimalPlaces>0 ? formatter.format(newBig(restProps.value)) : String(restProps.value);// after — formatter already carries the precisionconstformattedValue=formatter.format(newBig(restProps.value));
This removes duplicated state between formatter config and the explicit decimalPlaces param. Same simplification applies to the matching branch in marks.ts line 32.
⚠️ Low — Unconditional overlay JSX creation in createHandleRender
File:packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx line 30 Note:const overlay = <div>{tooltip?.value ?? ""}</div> is created on every handleRender call, even when isCustomText is false and the element is never used. Negligible cost, but worth moving inside the if (isCustomText) branch for clarity:
File:packages/pluggableWidgets/slider-web/src/Slider.editorPreview.tsx line 19 Note: The format function is typed as accepting value?: { toFixed: (n: number) => string } rather than Big. This works because Big satisfies that duck type, but it diverges from how the real NumberFormatter.format is typed and could mislead future editors. Consider annotating the parameter as Big | undefined to match production usage and the test-file mockFormatter.
Positives
Root-cause fix is precise: parseFloat(toFixed(n)).toString() was the exact line discarding trailing zeros, and the fix correctly routes formatting through the NumberFormatter which owns locale-aware decimal handling.
Using Big to wrap raw float values before passing to formatter.format avoids floating-point string artefacts (e.g. 4.600000000000001 → "4.60").
formatter is memoized in Container via useMemo with correct deps, so handleRender and useMarks only recompute when precision or the attribute's formatter changes — a nice performance improvement over the previous unmemoized version.
CHANGELOG entry is present, uses Keep a Changelog format, and is placed under [Unreleased] ✅
Test coverage is thorough: whole numbers, partial decimals, decimalPlaces=0, comma locale separators, and the edge cases (numberOfMarks=0, min===max) are all covered.
The mockFormatter factory in both test files correctly mirrors the real NumberFormatter contract without over-engineering.
⚠️ Low — Tooltip bypasses rounding when decimalPlaces === 0
File:src/utils/createHandleRender.tsx line 31 Problem:String(restProps.value) is used raw when decimalPlaces === 0. Mark labels round via parseFloat(rawValue.toFixed(0)), but the tooltip takes the RC Slider value as-is. If the slider ever yields a fractional value with decimalPlaces=0 (e.g., due to a mismatched step), the tooltip would display "9.5" while mark labels show "10" — a visible inconsistency. Fix: Pass through the formatter unconditionally and let it handle the precision:
This simplifies the branch and keeps tooltip and mark formatting consistent.
⚠️ Low — mockFormatter helper duplicated across test files
File:src/utils/__tests__/marks.spec.ts lines 5–19 and src/utils/__tests__/createHandleRender.spec.tsx lines 18–32 Problem: Identical mockFormatter function copied into both test files. Any future change (e.g., adding a withConfig implementation for a new test case) must be applied in two places. Fix: Extract to a shared test helper, e.g. src/utils/__tests__/testUtils.ts, and import from both spec files.
File:typings/declare-svg.ts lines 5–6 Problem:declare module "*.css" is semantically unrelated to SVG and was appended to the SVG ambient declaration file for convenience. It will confuse future readers. Fix: Move it to a separate typings/declare-css.ts or a general typings/global.d.ts.
⚠️ Low — withConfig in preview formatter silently ignores its argument
File:src/Slider.editorPreview.tsx lines 21 Problem:withConfig: () => previewFormatter always returns the same object regardless of the config argument passed in. This is currently harmless (nothing calls withConfig on this formatter after construction), but it means the formatter satisfies the NumberFormatter interface in a surprising way. A future maintainer adding a withConfig call through this code path would get silent incorrect behaviour. Fix: Add a comment to document the intentional stub, or implement it properly:
CHANGELOG entry is present and written in user-facing terms — exactly what belongs in an Unreleased section.
formatter is derived once via useMemo with correct deps (valueAttribute.formatter, decimalPlaces) — no redundant withConfig calls on every render.
handleRender is now memoized too, preventing unnecessary re-creation when unrelated props change — a welcome improvement over the pre-PR code.
new Big(rawValue) is used for formatter input, sidestepping floating-point precision artifacts in the displayed string even if rawValue has accumulated float error.
Test coverage is thorough: whole-number trailing zeros, partial decimal padding, locale decimal separators, decimalPlaces=0 backward compatibility, customText tooltip path, and both early-exit edge cases (numberOfMarks=0, min===max).
The decimalPlaces > 0 guard in marks correctly preserves the pre-existing integer label behavior without requiring a formatter round-trip.
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
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.
Pull request type
Bug fix (non-breaking change which fixes an issue)
Description
When
decimalPlacesis configured on the Slider widget, displayed values were silently stripping trailing zeros:10.00rendered as10,9.20rendered as9.2Root causes:
marks.ts—parseFloat(value.toFixed(n)).toString()round-tripped through a number, discarding trailing zeros. Fixed by keeping thetoFixedstring directly as the label and usingparseFloatonly for the numeric key.createHandleRender.tsx— tooltip overlay receivedrestProps.value(raw RC Slider number). Fixed by adding adecimalPlacesparameter and applying.toFixed(decimalPlaces)before passing to the overlay.Files changed:
src/utils/marks.ts— fix label generationsrc/utils/createHandleRender.tsx— accept and applydecimalPlacessrc/components/Container.tsx— passdecimalPlacestocreateHandleRendersrc/utils/__tests__/marks.spec.ts— new unit testssrc/utils/__tests__/createHandleRender.spec.tsx— new unit testsWhat should be covered while testing?
decimalPlaces = 2andNumber of markers > 0.0.00,5.00,10.00— not0,5,10.10.00, not10.9.50.decimalPlaces = 0and verify no change in existing behavior (labels and tooltip show integers without decimal point).