fix(slider): handle negative and decimal values in labels#2828
fix(slider): handle negative and decimal values in labels#2828Br1an67 wants to merge 1 commit intoAvaiga:developfrom
Conversation
Use parseFloat instead of parseInt to properly parse decimal label keys, and use undefined as sentinel instead of -1 to allow labels at position -1.
There was a problem hiding this comment.
Pull request overview
Fixes slider label parsing so marks can be created for negative and decimal positions (addressing #2586), avoiding truncation and the -1 “not found” sentinel collision.
Changes:
- Updated label-key parsing to use
parseFloat()andundefinedas the “not found” sentinel. - Added unit tests covering decimal, negative, and negative-decimal label keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/taipy-gui/src/components/Taipy/Slider.tsx | Adjusts marks/labels parsing to correctly accept negative and fractional numeric keys. |
| frontend/taipy-gui/src/components/Taipy/Slider.spec.tsx | Adds tests intended to validate mark rendering for decimal/negative label keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("shows marks with decimal values", async () => { | ||
| const labels = { | ||
| "0": "0", | ||
| "0.2": "0.2", | ||
| "0.4": "0.4", | ||
| "0.6": "0.6", | ||
| "0.8": "0.8", | ||
| "1": "1", | ||
| }; | ||
| const { container } = render(<Slider value={0.5} min={0} max={1} step={0.1} labels={JSON.stringify(labels)} />); | ||
| expect(container).toHaveTextContent("0"); | ||
| expect(container).toHaveTextContent("0.2"); | ||
| expect(container).toHaveTextContent("0.4"); | ||
| expect(container).toHaveTextContent("0.6"); | ||
| expect(container).toHaveTextContent("0.8"); | ||
| expect(container).toHaveTextContent("1"); | ||
| }); | ||
| it("shows marks with negative values including -1", async () => { | ||
| const labels = { | ||
| "-2": "-2", | ||
| "-1": "-1", | ||
| "0": "0", | ||
| "1": "1", | ||
| "2": "2", | ||
| }; | ||
| const { container } = render(<Slider value={0} min={-2} max={2} step={1} labels={JSON.stringify(labels)} />); | ||
| expect(container).toHaveTextContent("-2"); | ||
| expect(container).toHaveTextContent("-1"); | ||
| expect(container).toHaveTextContent("0"); | ||
| expect(container).toHaveTextContent("1"); | ||
| expect(container).toHaveTextContent("2"); | ||
| }); | ||
| it("shows marks with negative decimal values", async () => { | ||
| const labels = { | ||
| "-1.0": "-1.0", | ||
| "-0.5": "-0.5", | ||
| "0": "0", | ||
| "0.5": "0.5", | ||
| "1.0": "1.0", | ||
| }; | ||
| const { container } = render(<Slider value={0} min={-1} max={1} step={0.5} labels={JSON.stringify(labels)} />); | ||
| expect(container).toHaveTextContent("-1.0"); | ||
| expect(container).toHaveTextContent("-0.5"); | ||
| expect(container).toHaveTextContent("0"); | ||
| expect(container).toHaveTextContent("0.5"); | ||
| expect(container).toHaveTextContent("1.0"); | ||
| }); |
There was a problem hiding this comment.
These new tests are declared async but don’t await anything. Removing async avoids misleading signal about asynchronous behavior and prevents accidental swallowed assertion timing issues.
| const { container } = render(<Slider value={0.5} min={0} max={1} step={0.1} labels={JSON.stringify(labels)} />); | ||
| expect(container).toHaveTextContent("0"); | ||
| expect(container).toHaveTextContent("0.2"); | ||
| expect(container).toHaveTextContent("0.4"); | ||
| expect(container).toHaveTextContent("0.6"); | ||
| expect(container).toHaveTextContent("0.8"); | ||
| expect(container).toHaveTextContent("1"); |
There was a problem hiding this comment.
The new decimal-label test only asserts that the label text exists in the DOM; it doesn’t verify that marks are placed at distinct positions/values. This test would likely still pass in the pre-fix behavior where decimal keys were truncated (marks rendered but grouped at the same tick). Consider asserting mark placement (e.g., querying mark label elements and checking their computed left/bottom percentages, or verifying the style/position differs per label).
| const { container } = render(<Slider value={0.5} min={0} max={1} step={0.1} labels={JSON.stringify(labels)} />); | |
| expect(container).toHaveTextContent("0"); | |
| expect(container).toHaveTextContent("0.2"); | |
| expect(container).toHaveTextContent("0.4"); | |
| expect(container).toHaveTextContent("0.6"); | |
| expect(container).toHaveTextContent("0.8"); | |
| expect(container).toHaveTextContent("1"); | |
| const { container, getByText } = render( | |
| <Slider value={0.5} min={0} max={1} step={0.1} labels={JSON.stringify(labels)} /> | |
| ); | |
| expect(container).toHaveTextContent("0"); | |
| expect(container).toHaveTextContent("0.2"); | |
| expect(container).toHaveTextContent("0.4"); | |
| expect(container).toHaveTextContent("0.6"); | |
| expect(container).toHaveTextContent("0.8"); | |
| expect(container).toHaveTextContent("1"); | |
| // Ensure that the marks corresponding to decimal values are positioned distinctly. | |
| const labelElements = Object.values(labels).map((text) => getByText(text)); | |
| const leftPositions = labelElements.map((elt) => (elt as HTMLElement).style.left); | |
| const distinctLeftPositions = new Set(leftPositions); | |
| // If all labels were rendered at the same tick, all left positions would be identical (or all empty). | |
| expect(distinctLeftPositions.size).toBeGreaterThan(1); |
FredLL-Avaiga
left a comment
There was a problem hiding this comment.
Excellent @Br1an67
Thank you
can you change parseFloat to Number ?
| if (lovIdx !== -1) { | ||
| idx = lovIdx; | ||
| } else { | ||
| const num = parseFloat(key); |
There was a problem hiding this comment.
I'd prefer to use Number(key)
|
@Br1an67 can you reply to copilot ? |
4e0a386 to
f21c998
Compare
Fixes #2586
What type of PR is this? (Check all that apply)
Description
Fixed the slider labels functionality to properly handle negative and decimal values. The issue was caused by two problems in the label parsing logic:
parseInt(key, 10)was used instead ofparseFloat(key), causing decimal values like0.2,0.4to be truncated to0-1was used as a sentinel value for "not found", which prevented labels at position-1from being displayedThe fix uses
parseFloatfor parsing andundefinedas the sentinel value instead of-1.Related Tickets & Documents
How to reproduce the issue
Or with negative values:
Backporting
This change should be backported to:
Checklist
Changes