Skip to content

fix: round to correct precision when step uses exponential notation #8290

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.

Sign up for GitHub

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
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: round to correct precision when step uses exponential notation #8290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2d7658
fix: round to correct precision when step uses exponential notation
lucasweng May 22, 2025
872c527
fix: correct significand precision calculation for exponential steps
lucasweng Jun 2, 2025
96317b5
Merge remote-tracking branch 'upstream/main' into issues-8274
lucasweng Jun 2, 2025
afa153f
Merge branch 'main' into issues-8274
snowystinger Jun 4, 2025
c447134
refactor: simplify precision calculation for exponents
lucasweng Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/@react-stately/utils/src/number.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,18 @@ export function clamp(value: number, min: number = -Infinity, max: number = Infi

export function roundToStepPrecision(value: number, step: number): number {
let roundedValue = value;
let precision = 0;
let stepString = step.toString();
let pointIndex = stepString.indexOf('.');
let precision = pointIndex >= 0 ? stepString.length - pointIndex : 0;
// Handle negative exponents in exponential notation (e.g., "1e-7" → precision 8)
let eIndex = stepString.toLowerCase().indexOf('e-');
if (eIndex > 0) {
precision = Math.abs(Math.floor(Math.log10(Math.abs(step)))) + eIndex;
} else {
let pointIndex = stepString.indexOf('.');
if (pointIndex >= 0) {
precision = stepString.length - pointIndex;
}
}
if (precision > 0) {
let pow = Math.pow(10, precision);
roundedValue = Math.round(roundedValue * pow) / pow;
Expand Down
30 changes: 29 additions & 1 deletion packages/@react-stately/utils/test/number.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {clamp, snapValueToStep} from '../src/number';
import {clamp, roundToStepPrecision, snapValueToStep} from '../src/number';

describe('number utils', () => {
describe('clamp', () => {
Expand All @@ -8,6 +8,34 @@ describe('number utils', () => {
});
});

describe('roundToStepPrecision', () => {
it('should return the input unchanged for integer steps', () => {
expect(roundToStepPrecision(7.123, 1)).toBe(7.123);
expect(roundToStepPrecision(5, 10)).toBe(5);
});
it('should round to the correct decimal places for steps with decimals', () => {
expect(roundToStepPrecision(1.24, 0.1)).toBe(1.24);
Copy link
Member

@snowystinger snowystinger May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

maybe I'm over thinking this, but shouldn't this be

Suggested change
expect(roundToStepPrecision(1.24, 0.1)).toBe(1.24);
expect(roundToStepPrecision(1.24, 0.1)).toBe(1.2);

I realize it's the existing behaviour, I'll have to look back to remember why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into this! I was wondering the same thing while looking at the original implementation 🤔

let precision = pointIndex >= 0 ? stepString.length - pointIndex : 0;

I also thought precision should maybe be stepString.length - pointIndex - 1, but the original behavior seemed correct, since all the existing test cases in NumberField.test and number.test passed. Also, this #8274 (comment) suggests that for an exponential notation like 1e-7, the precision should be 8. So, I implemented the existing behavior for exponential notation as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @scrgl thanks for pointing out that incorrect implementation! 872c527 fixed the issue mentioned in #8290 (comment) and added more tests.

I opted not to use split as suggested in the snippet because it creates additional intermediate arrays every time snapValueToStep is called. Since this function may be invoked multiple times by hooks and components, those arrays could quickly add up. Also, since step values without explicit exponential notation are likely more common in user land, normalizing everything to exponential format upfront would add extra overhead for those cases.

expect(roundToStepPrecision(1.456, 0.01)).toBe(1.456);
expect(roundToStepPrecision(1.2345, 0.015)).toBe(1.2345);
expect(roundToStepPrecision(1.2345, 0.25)).toBe(1.235);
// Should not overcount precision
expect(roundToStepPrecision(2.349, 0.100)).toBe(2.35);
expect(roundToStepPrecision(2.35, 0.100)).toBe(2.35);
// Should handle negative values
expect(roundToStepPrecision(-1.456, 0.01)).toBe(-1.456);
// Should handle zero value
expect(roundToStepPrecision(0, 0.01)).toBe(0);
});
it('should handle rounding for exponential step values', () => {
expect(roundToStepPrecision(0.123456789, 1e-3)).toBe(0.1235);
expect(roundToStepPrecision(0.123456789, 1e-7)).toBe(0.12345679);
expect(roundToStepPrecision(0.123456789, 1.5e-7)).toBe(0.123456789);
expect(roundToStepPrecision(0.123456789, 2.5e-6)).toBe(0.12345679);
// Should handle exponential notation steps regardless of e/E case
expect(roundToStepPrecision(0.123456789, 1E-8)).toBe(0.123456789);
});
});

describe('snapValueToStep', () => {
it('should snap value to nearest step based on min and max', () => {
expect(snapValueToStep(2, -0.5, 100, 3)).toBe(2.5);
Expand Down