-
Notifications
You must be signed in to change notification settings - Fork 78
[WC-3322]: Fix slider decimal places formatting #2220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
samuelreichert
wants to merge
6
commits into
main
Choose a base branch
from
worktree-fix+slider-decimal-places-formatting
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9bdf274
fix(slider-web): declare CSS module type to fix side-effect import error
samuelreichert d04c366
fix(slider-web): format marks and tooltip with locale-aware decimal s…
samuelreichert c9f0a7a
test(slider-web): update unit and E2E tests for decimal formatting
samuelreichert b8d4680
chore(slider-web): add changelog entry for decimal places formatting fix
samuelreichert d616b79
refactor(slider-web): use built-in NumberFormatter for marks and tooltip
samuelreichert f4aefab
fix(changelog): update description for decimal places and locale form…
samuelreichert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
81 changes: 81 additions & 0 deletions
81
packages/pluggableWidgets/slider-web/src/utils/__tests__/createHandleRender.spec.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { SliderProps as RcSliderProps } from "@rc-component/slider"; | ||
| import { createRef, ReactElement } from "react"; | ||
| import { createHandleRender } from "../createHandleRender"; | ||
|
|
||
| const defaultRenderProps = { | ||
| dragging: false, | ||
| index: 0, | ||
| prefixCls: "rc-slider-handle", | ||
| draggingDelete: false, | ||
| onFocus: jest.fn(), | ||
| onBlur: jest.fn() | ||
| }; | ||
|
|
||
| const mockNode = <div />; | ||
|
|
||
| // Deterministic stand-in for createValueFormatter's output. | ||
| const formatWith = | ||
| (decimalPlaces: number, decimalSeparator = ".") => | ||
| (value: number): string => { | ||
| const fixed = value.toFixed(decimalPlaces); | ||
| return decimalSeparator === "." ? fixed : fixed.replace(".", decimalSeparator); | ||
| }; | ||
|
|
||
| function buildHandleRender( | ||
| decimalPlaces: number, | ||
| tooltipType: "value" | "customText" = "value", | ||
| decimalSeparator = "." | ||
| ): NonNullable<RcSliderProps["handleRender"]> { | ||
| const sliderRef = createRef<HTMLDivElement>(); | ||
| return createHandleRender({ | ||
| tooltipType, | ||
| tooltipAlwaysVisible: true, | ||
| sliderRef, | ||
| format: formatWith(decimalPlaces, decimalSeparator) | ||
| })!; | ||
| } | ||
|
|
||
| describe("createHandleRender tooltip value formatting", () => { | ||
| it("formats whole number with trailing zeros when decimalPlaces=2", () => { | ||
| const result = buildHandleRender(2)(mockNode, { ...defaultRenderProps, value: 10 } as any) as ReactElement<any>; | ||
| expect(result.props.overlay).toBe("10.00"); | ||
| }); | ||
|
|
||
| it("formats partial decimal with trailing zero when decimalPlaces=2", () => { | ||
| const result = buildHandleRender(2)(mockNode, { | ||
| ...defaultRenderProps, | ||
| value: 9.2 | ||
| } as any) as ReactElement<any>; | ||
| expect(result.props.overlay).toBe("9.20"); | ||
| }); | ||
|
|
||
| it("formats value without decimals when decimalPlaces=0", () => { | ||
| const result = buildHandleRender(0)(mockNode, { ...defaultRenderProps, value: 10 } as any) as ReactElement<any>; | ||
| expect(result.props.overlay).toBe("10"); | ||
| }); | ||
|
|
||
| it("uses locale decimal separator", () => { | ||
| const result = buildHandleRender( | ||
| 2, | ||
| "value", | ||
| "," | ||
| )(mockNode, { | ||
| ...defaultRenderProps, | ||
| value: 9.2 | ||
| } as any) as ReactElement<any>; | ||
| expect(result.props.overlay).toBe("9,20"); | ||
| }); | ||
|
|
||
| it("renders custom text tooltip ignoring value formatting", () => { | ||
| const sliderRef = createRef<HTMLDivElement>(); | ||
| const handleRender = createHandleRender({ | ||
| tooltip: { value: "custom label" } as any, | ||
| tooltipType: "customText", | ||
| tooltipAlwaysVisible: true, | ||
| sliderRef, | ||
| format: formatWith(2) | ||
| })!; | ||
| const result = handleRender(mockNode, { ...defaultRenderProps, value: 10 } as any) as ReactElement<any>; | ||
| expect(result.props.overlay.props.children).toBe("custom label"); | ||
| }); | ||
| }); |
62 changes: 62 additions & 0 deletions
62
packages/pluggableWidgets/slider-web/src/utils/__tests__/helpers.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import { Big } from "big.js"; | ||
| import { NumberFormatter } from "mendix"; | ||
| import { createValueFormatter } from "../helpers"; | ||
|
|
||
| /** | ||
| * Minimal stand-in for the Mendix runtime NumberFormatter. It mimics the two behaviours the | ||
| * widget relies on: `withConfig` returns a new formatter with the merged config, and `format` | ||
| * honours `decimalPrecision`, `groupDigits` and the configured locale separators. | ||
| */ | ||
| function fakeNumberFormatter( | ||
| config: { groupDigits: boolean; decimalPrecision?: number }, | ||
| locale: { decimal: string; group: string } = { decimal: ".", group: "," } | ||
| ): NumberFormatter { | ||
| return { | ||
| type: "number", | ||
| config, | ||
| withConfig: (next: { groupDigits: boolean; decimalPrecision?: number }) => | ||
| fakeNumberFormatter({ ...config, ...next }, locale), | ||
| format: (value?: Big) => { | ||
| if (value == null) { | ||
| return ""; | ||
| } | ||
| const fixed = value.toNumber().toFixed(config.decimalPrecision ?? 0); | ||
| const [intPart, fracPart] = fixed.split("."); | ||
| const grouped = config.groupDigits ? intPart.replace(/\B(?=(\d{3})+(?!\d))/g, locale.group) : intPart; | ||
| return fracPart != null ? `${grouped}${locale.decimal}${fracPart}` : grouped; | ||
| }, | ||
| parse: () => ({ valid: false }) | ||
| } as unknown as NumberFormatter; | ||
| } | ||
|
|
||
| describe("createValueFormatter", () => { | ||
| it("redefines the formatter's decimal precision (forces trailing zeros)", () => { | ||
| const format = createValueFormatter(fakeNumberFormatter({ groupDigits: false }), 2); | ||
| expect(format(10)).toBe("10.00"); | ||
| expect(format(9.2)).toBe("9.20"); | ||
| }); | ||
|
|
||
| it("formats without decimals when decimalPlaces is 0", () => { | ||
| const format = createValueFormatter(fakeNumberFormatter({ groupDigits: false }), 0); | ||
| expect(format(10)).toBe("10"); | ||
| expect(format(9.7)).toBe("10"); | ||
| }); | ||
|
|
||
| it("respects the locale decimal separator from the formatter", () => { | ||
| const format = createValueFormatter( | ||
| fakeNumberFormatter({ groupDigits: false }, { decimal: ",", group: "." }), | ||
| 2 | ||
| ); | ||
| expect(format(9.2)).toBe("9,20"); | ||
| }); | ||
|
|
||
| it("keeps the attribute's thousands grouping when groupDigits is enabled", () => { | ||
| const format = createValueFormatter(fakeNumberFormatter({ groupDigits: true }), 0); | ||
| expect(format(1000000)).toBe("1,000,000"); | ||
| }); | ||
|
|
||
| it("omits thousands grouping when groupDigits is disabled", () => { | ||
| const format = createValueFormatter(fakeNumberFormatter({ groupDigits: false }), 0); | ||
| expect(format(1000000)).toBe("1000000"); | ||
| }); | ||
| }); |
74 changes: 74 additions & 0 deletions
74
packages/pluggableWidgets/slider-web/src/utils/__tests__/marks.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { createMarks } from "../marks"; | ||
|
|
||
| // Simple deterministic formatter standing in for createValueFormatter's output. | ||
| const formatWith = | ||
| (decimalPlaces: number, decimalSeparator = ".") => | ||
| (value: number): string => { | ||
| const fixed = value.toFixed(decimalPlaces); | ||
| return decimalSeparator === "." ? fixed : fixed.replace(".", decimalSeparator); | ||
| }; | ||
|
|
||
| describe("createMarks", () => { | ||
| it("forces trailing zeros when decimalPlaces > 0 and value is whole number", () => { | ||
| const marks = createMarks({ numberOfMarks: 2, decimalPlaces: 2, format: formatWith(2), min: 0, max: 10 }); | ||
| expect(marks).toBeDefined(); | ||
| expect(marks![0]).toBe("0.00"); | ||
| expect(marks![5]).toBe("5.00"); | ||
| expect(marks![10]).toBe("10.00"); | ||
| }); | ||
|
|
||
| it("forces trailing zeros when decimalPlaces > 0 and value has fewer decimals", () => { | ||
| const marks = createMarks({ numberOfMarks: 2, decimalPlaces: 2, format: formatWith(2), min: 0, max: 9.2 }); | ||
| expect(marks).toBeDefined(); | ||
| expect(marks![4.6]).toBe("4.60"); | ||
| expect(marks![9.2]).toBe("9.20"); | ||
| }); | ||
|
|
||
| it("does not add decimal places when decimalPlaces is 0", () => { | ||
| const marks = createMarks({ numberOfMarks: 4, decimalPlaces: 0, format: formatWith(0), min: 0, max: 100 }); | ||
| expect(marks).toBeDefined(); | ||
| expect(marks![0]).toBe("0"); | ||
| expect(marks![25]).toBe("25"); | ||
| expect(marks![100]).toBe("100"); | ||
| }); | ||
|
|
||
| it("uses locale decimal separator", () => { | ||
| const marks = createMarks({ numberOfMarks: 2, decimalPlaces: 2, format: formatWith(2, ","), min: 0, max: 10 }); | ||
| expect(marks![0]).toBe("0,00"); | ||
| expect(marks![5]).toBe("5,00"); | ||
| expect(marks![10]).toBe("10,00"); | ||
| }); | ||
|
|
||
| it("uses correct numeric keys for fractional values with comma locale", () => { | ||
| const marks = createMarks({ numberOfMarks: 2, decimalPlaces: 2, format: formatWith(2, ","), min: 0, max: 9.2 }); | ||
| expect(marks![4.6]).toBe("4,60"); | ||
| expect(marks![9.2]).toBe("9,20"); | ||
| }); | ||
|
|
||
| it("rounds mark keys to the configured decimal places so dots align with their labels", () => { | ||
| // 9 intervals over 0..20 yields repeating decimals (e.g. 6.6667). The key must be the | ||
| // rounded value (6.7) so rc-slider positions the dot where the label reads. | ||
| const marks = createMarks({ numberOfMarks: 9, decimalPlaces: 1, format: formatWith(1), min: 0, max: 20 }); | ||
| expect(Object.keys(marks!)).toContain("6.7"); | ||
| expect(Object.keys(marks!)).not.toContain("6.666666666666667"); | ||
| expect(marks![6.7]).toBe("6.7"); | ||
| }); | ||
|
|
||
| it("returns undefined when numberOfMarks is 0", () => { | ||
| expect( | ||
| createMarks({ numberOfMarks: 0, decimalPlaces: 2, format: formatWith(2), min: 0, max: 100 }) | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("returns undefined when min equals max", () => { | ||
| expect( | ||
| createMarks({ numberOfMarks: 4, decimalPlaces: 2, format: formatWith(2), min: 5, max: 5 }) | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("returns undefined when min > max", () => { | ||
| expect( | ||
| createMarks({ numberOfMarks: 2, decimalPlaces: 1, format: formatWith(1), min: 10, max: 5 }) | ||
| ).toBeUndefined(); | ||
| }); | ||
| }); | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why this pattern, and not
marks?.[0]?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 14 we already check if marks is defined