Skip to content

[WC-3322]: Fix slider decimal places formatting#2220

Open
samuelreichert wants to merge 3 commits into
mainfrom
worktree-fix+slider-decimal-places-formatting
Open

[WC-3322]: Fix slider decimal places formatting#2220
samuelreichert wants to merge 3 commits into
mainfrom
worktree-fix+slider-decimal-places-formatting

Conversation

@samuelreichert
Copy link
Copy Markdown
Contributor

Pull request type

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:

  1. marks.tsparseFloat(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.

  2. 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?

  1. Configure a Slider widget with decimalPlaces = 2 and Number of markers > 0.
  2. Set min/max so some markers land on whole numbers (e.g. min=0, max=10, markers=2).
  3. Mark labels should show 0.00, 5.00, 10.00 — not 0, 5, 10.
  4. Drag the handle to a whole number value — tooltip should show 10.00, not 10.
  5. Drag to a value with one decimal (e.g. 9.5) — tooltip should show 9.50.
  6. Set decimalPlaces = 0 and verify no change in existing behavior (labels and tooltip show integers without decimal point).

@samuelreichert samuelreichert requested a review from a team as a code owner May 19, 2026 11:35
@github-actions

This comment was marked as outdated.

@samuelreichert samuelreichert changed the title Worktree fix+slider decimal places formatting [WC-3322]: Fix slider decimal places formatting May 19, 2026
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch from 4e14487 to 945997f Compare May 22, 2026 12:23
@github-actions

This comment was marked as outdated.

@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch from 945997f to 98f491a Compare May 26, 2026 07:37
@github-actions

This comment was marked as outdated.

@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch 3 times, most recently from 07fc177 to 691462a Compare May 26, 2026 13:20
@github-actions

This comment was marked as outdated.

iobuhov
iobuhov previously approved these changes May 26, 2026
Copy link
Copy Markdown
Collaborator

@iobuhov iobuhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Outdated
@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch from fce3bc3 to 955eda7 Compare May 28, 2026 10:49
@github-actions
Copy link
Copy Markdown

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/slider-web/CHANGELOG.md Added ### Fixed entry for decimal-places formatting
packages/pluggableWidgets/slider-web/src/components/Container.tsx Wrapped createHandleRender call in useMemo; added decimalPlaces prop
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Added decimalPlaces param; uses formatDecimal for tooltip value
packages/pluggableWidgets/slider-web/src/utils/helpers.ts New formatDecimal utility with locale-aware decimal separator
packages/pluggableWidgets/slider-web/src/utils/marks.ts Use formatDecimal for label; parseFloat(label) for numeric key
packages/pluggableWidgets/slider-web/typings/declare-svg.ts Added declare module "*.css"
packages/pluggableWidgets/slider-web/typings/global.d.ts New file — typed window.mx locale shape
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts New unit tests: locale separator + trailing-zero cases
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx 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:

for (let i = 0; i <= numberOfMarks; i++) {
    const rawValue = min + i * interval;
    const key = parseFloat(rawValue.toFixed(decimalPlaces)); // always uses dot
    const label = formatDecimal(rawValue, decimalPlaces);
    marks[key] = label;
}

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.

@samuelreichert samuelreichert requested review from iobuhov and r0b1n May 28, 2026 10:55
@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch from 955eda7 to 0fbf863 Compare May 28, 2026 13:16
@github-actions
Copy link
Copy Markdown

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/slider-web/CHANGELOG.md Added ### Fixed entry under [Unreleased]
packages/pluggableWidgets/slider-web/src/Slider.editorPreview.tsx Added inline previewFormatter to satisfy new formatter param
packages/pluggableWidgets/slider-web/src/components/Container.tsx Added useMemo for formatter and handleRender; passes formatter to both
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Accepts formatter+decimalPlaces; formats tooltip via formatter.format(new Big(value))
packages/pluggableWidgets/slider-web/src/utils/marks.ts Replaces parseFloat(...).toString() with formatter.format(new Big(rawValue)) for labels
packages/pluggableWidgets/slider-web/src/utils/useMarks.ts Threads formatter through to createMarks; adds it to useMemo deps
packages/pluggableWidgets/slider-web/typings/declare-svg.ts Adds declare module "*.css" stub
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx New unit tests for tooltip formatting including locale separator
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts 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:

// before
const formattedValue = decimalPlaces > 0 ? formatter.format(new Big(restProps.value)) : String(restProps.value);

// after — formatter already carries the precision
const formattedValue = formatter.format(new Big(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:

const overlay = isCustomText ? <div>{tooltip?.value ?? ""}</div> : null;

⚠️ Low — previewFormatter.format uses duck-typed input

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.

@samuelreichert samuelreichert force-pushed the worktree-fix+slider-decimal-places-formatting branch from 0fbf863 to 457a610 Compare May 28, 2026 15:12
@github-actions
Copy link
Copy Markdown

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/slider-web/CHANGELOG.md Unreleased entry for the decimal-places fix
packages/pluggableWidgets/slider-web/src/Slider.editorPreview.tsx Added stub previewFormatter, passes formatter to createMarks
packages/pluggableWidgets/slider-web/src/components/Container.tsx Derives NumberFormatter via useMemo; memoizes handleRender; passes formatter downstream
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Accepts formatter/decimalPlaces; formats tooltip via formatter.format
packages/pluggableWidgets/slider-web/src/utils/marks.ts Accepts formatter; uses formatter.format(new Big(rawValue)) for mark labels
packages/pluggableWidgets/slider-web/src/utils/useMarks.ts Threads formatter through to createMarks
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx New unit tests: trailing zeros, partial decimals, locale separator, custom-text path
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts New unit tests: trailing zeros, locale separator, decimalPlaces=0, edge cases
packages/pluggableWidgets/slider-web/typings/declare-svg.ts Added declare module "*.css"

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

⚠️ 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:

const formattedValue = formatter.format(new Big(restProps.value));

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.


⚠️ Low — CSS module declaration added to SVG declaration file

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:

withConfig: (cfg) => mockFormatter(cfg.decimalPrecision ?? decimalPrecision)

Positives

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants