Skip to content

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

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

[WC-3322]: Fix slider decimal places formatting#2220
samuelreichert wants to merge 5 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
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 added for the decimal-places bug fix
packages/pluggableWidgets/slider-web/src/components/Container.tsx Passes decimalPlaces to createHandleRender
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Accepts decimalPlaces and applies .toFixed() to tooltip value
packages/pluggableWidgets/slider-web/src/utils/marks.ts Uses toFixed string directly as label; parseFloat only for the numeric key
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx New unit tests for tooltip formatting
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts New unit tests for mark label formatting

CI checks could not be fetched (tool permission). No dist/, lockfile, or generated files changed.


Findings

⚠️ Low — createHandleRender called without useMemo in InnerContainer

File: packages/pluggableWidgets/slider-web/src/components/Container.tsx line 33–41
Note: createHandleRender(...) is called unconditionally in the render body. This was already the case before this PR, but the new decimalPlaces parameter makes it worth flagging: a new function reference is created on every render, which can cause RC Slider to re-create the tooltip unnecessarily. Wrapping in useMemo would align with how useMarks is already handled.

const handleRender = useMemo(
    () =>
        props.showTooltip
            ? createHandleRender({
                  tooltip: props.tooltip,
                  tooltipType: props.tooltipType,
                  tooltipAlwaysVisible: props.tooltipAlwaysVisible,
                  sliderRef,
                  decimalPlaces: props.decimalPlaces
              })
            : undefined,
    [props.showTooltip, props.tooltip, props.tooltipType, props.tooltipAlwaysVisible, props.decimalPlaces]
);

⚠️ Low — Test files lack a defaultProps factory helper; sliderRef duplicated in every test case

File: packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx
Note: Each of the four test cases constructs its own sliderRef, mockNode, and full mockProps from scratch. Extracting shared boilerplate into a defaultRenderProps constant and a buildHandleRender helper would reduce repetition and make future test additions easier. Not blocking, but the pattern is noticeable.


⚠️ Low — marks.spec.ts missing edge-case coverage for min === max

File: packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts
Note: isParamsValidToCalcMarks guards min < max, so createMarks({ ..., min: 5, max: 5 }) returns undefined. A test for this boundary would complete the guard coverage alongside the existing numberOfMarks: 0 test.


Positives

  • Root cause analysis in the PR description is precise and accurate — clearly identifies the two separate bugs and which files contained each one.
  • The marks.ts fix is minimal and correct: reusing the toFixed string as the label avoids the round-trip through parseFloat(...).toString() that was dropping trailing zeros.
  • Using parseFloat(label) for the numeric key preserves RC Slider's requirement for a number key while keeping the string label — a clean separation.
  • New tests cover the exact scenarios described in the bug report (whole numbers, partial decimals, zero decimal places) and a customText tooltip branch that would otherwise be easy to regress.
  • CHANGELOG entry follows Keep a Changelog format and is written in user-facing language.

@samuelreichert samuelreichert changed the title Worktree fix+slider decimal places formatting [WC-3322]: Fix slider decimal places formatting May 19, 2026
@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/src/utils/marks.ts Fix label generation — keep toFixed string as label, use parseFloat only for the numeric key
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Accept decimalPlaces parameter, apply .toFixed() before passing to tooltip overlay
packages/pluggableWidgets/slider-web/src/components/Container.tsx Pass decimalPlaces to createHandleRender; wrap call in useMemo
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts New unit tests for createMarks covering trailing zeros and edge cases
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx New unit tests for tooltip value formatting
packages/pluggableWidgets/slider-web/CHANGELOG.md Unreleased Fixed entry added

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

⚠️ CI check status could not be verifiedgh pr checks required additional approval. Please confirm all checks are green before merging.


Findings

⚠️ Low — formattedValue computed unconditionally in createHandleRender

File: packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx line 28
Note: restProps.value.toFixed(decimalPlaces) runs on every render even when isCustomText is true, where the result is never used. The computation is cheap and won't cause a bug, but it's slightly wasteful.
Fix (optional):

const formattedValue = isCustomText ? "" : restProps.value.toFixed(decimalPlaces);

Or inline directly:

overlay={isCustomText ? overlay : restProps.value.toFixed(decimalPlaces)}

Positives

  • Core fix in marks.ts is minimal and precise: preserving toFixed string as the label while using parseFloat only for the RC Slider numeric key is exactly the right approach.
  • useMemo addition in Container.tsx is a good bonus — avoids recreating the render function on every render; sliderRef is correctly excluded from the dep array since ref objects are stable.
  • Tests cover all the meaningful branches: whole-number trailing zeros, partial-decimal trailing zeros, decimalPlaces=0 regression, numberOfMarks=0 and min===max guard cases, and custom-text tooltip bypassing formatting.
  • CHANGELOG entry is user-facing and concrete — cites the exact values that change (1010.00, 9.29.20).

@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 [Unreleased] > Fixed entry for decimal formatting bug
packages/pluggableWidgets/slider-web/src/components/Container.tsx Wrapped createHandleRender call in useMemo; added decimalPlaces prop pass-through
packages/pluggableWidgets/slider-web/src/utils/createHandleRender.tsx Added decimalPlaces param; apply .toFixed() on tooltip overlay value
packages/pluggableWidgets/slider-web/src/utils/marks.ts Fixed label generation to keep toFixed string directly; use parseFloat only for the key
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx New unit tests for tooltip formatting
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts New unit tests for mark label generation

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


Findings

⚠️ Low — useMemo deps array missing sliderRef

File: packages/pluggableWidgets/slider-web/src/components/Container.tsx line 44
Note: sliderRef is captured by createHandleRender via closure and used inside the returned render function, but it's omitted from the useMemo dependency array. Because sliderRef is a stable useRef object that never changes identity, this is harmless in practice — but linters (and reviewers) will flag it. Consider either adding sliderRef to the array (no functional cost) or suppressing the exhaustive-deps lint rule with a comment.


⚠️ Low — No test for decimalPlaces received as undefined/null from Mendix runtime

File: packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx
Note: SliderContainerProps types decimalPlaces as number (non-nullable), but the generated preview props type it as number | null, and the editorPreview uses ?? 2 as a fallback. The runtime path goes through the non-nullable type, so this is unlikely to cause a real bug — but a test case covering decimalPlaces=0 for mark labels (i.e. "0" not "0.00") already exists, and a case for decimalPlaces=1 would round out coverage.


Positives

  • Root cause analysis in the PR description is precise — correctly identifies two separate call sites (marks.ts and createHandleRender.tsx) and explains the parseFloat(toFixed(...)).toString() round-trip loss.
  • marks.ts fix is minimal and correct: separating label (string) from key (number) is the right model.
  • useMemo wrapping of createHandleRender is a good bonus improvement that prevents a new closure being created on every render.
  • Tests are written with describe/it structure, cover all branching paths (whole number, partial decimal, zero decimals, custom text tooltip), and use no snapshots.
  • CHANGELOG entry is present and follows the Keep a Changelog format.

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.

1 participant