Skip to content
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

fix(number-field, slider): floating point roundoff precision bug #4263

Merged
merged 30 commits into from Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3f6fde4
fix(number-field): fixed precision bug
blunteshwar Apr 11, 2024
c8a49bc
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 12, 2024
15bf141
chore(number-field): some minor fix
blunteshwar Apr 12, 2024
85bcd8e
chore(slider, number-field): added some tests and story
blunteshwar Apr 12, 2024
87f073b
Apply suggestions from code review
blunteshwar Apr 15, 2024
763c8d1
Revert "Apply suggestions from code review"
blunteshwar Apr 15, 2024
831ebbb
chore(number-field): updated test name
blunteshwar Apr 15, 2024
5f4fc2e
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 17, 2024
dd6ee09
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
c0675ae
fix(number-field): number formatter to format decimal values
blunteshwar Apr 29, 2024
5b080da
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
3d5f8af
chore(number-field): minor fix
blunteshwar Apr 29, 2024
81f20ca
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
947091b
chore(number-field): implemented formatter using getter
blunteshwar Apr 29, 2024
53e621a
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
8b52e92
chore(number-field): implemented single number formatter
blunteshwar Apr 29, 2024
88a4a4d
chore(number-field): some minor fix
blunteshwar Apr 29, 2024
8ce6361
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
a21b404
chore(number-field): minor fixes
blunteshwar Apr 29, 2024
e077c20
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
d0b8d53
chore(number-field): implemented an independent formatter
blunteshwar Apr 29, 2024
70b6b14
chore(number-field): minor change
blunteshwar Apr 29, 2024
395e004
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
14cbe9e
chore(number-field): cached digitsAfterDecimal
blunteshwar Apr 29, 2024
d4affc5
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
207ec62
chore(number-field): some minor changes
blunteshwar Apr 29, 2024
f5ad973
chore(number-field): minor fix
blunteshwar Apr 29, 2024
05f8bf9
chore(number-field): minor ts fix
blunteshwar Apr 29, 2024
c8e2faf
chore(number-field): minor ts check fix
blunteshwar Apr 29, 2024
b218046
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 33 additions & 4 deletions packages/number-field/src/NumberField.ts
Expand Up @@ -495,7 +495,14 @@ export class NumberField extends TextfieldBase {
// Step shouldn't validate when 0...
if (this.step) {
const min = typeof this.min !== 'undefined' ? this.min : 0;
const moduloStep = (value - min) % this.step;
this.digitsAfterDecimal =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we cauterize this work to change.get('step') in one of the lifecycle callbacks? I'm not super excited about the this._lastDigitsAfterDecimal !== this.digitsAfterDecimal check, can we leverage the existing cache management mechanics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

this.step != Math.floor(this.step)
? this.step.toString().split('.')[1].length
Copy link
Contributor

Choose a reason for hiding this comment

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

If this.step is not a floating-point number, calling split('.')[1] on it will result in an error. Check first if this.step is a floating point number or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using a ternary operator where if this.step is not a floating-point number, then this.step != Math.floor(this.step) will evaluate to false and hence digitsAfterDecimals will be assigned 0. (Line 501)

: 0;
const moduloStep = parseFloat(
this.numberFormatter.format((value - min) % this.step)
);

const fallsOnStep = moduloStep === 0;
if (!fallsOnStep) {
const overUnder = Math.round(moduloStep / this.step);
Expand All @@ -510,6 +517,7 @@ export class NumberField extends TextfieldBase {
value -= this.step;
}
}
value = parseFloat(this.numberFormatter.format(value));
}
value *= signMultiplier;
return value;
Expand All @@ -526,7 +534,11 @@ export class NumberField extends TextfieldBase {
}

protected get numberFormatter(): NumberFormatter {
if (!this._numberFormatter || !this._numberFormatterFocused) {
if (
!this._numberFormatter ||
!this._numberFormatterFocused ||
this._lastDigitsAfterDecimal !== this.digitsAfterDecimal
) {
const {
style,
unit,
Expand All @@ -539,12 +551,26 @@ export class NumberField extends TextfieldBase {
}
this._numberFormatterFocused = new NumberFormatter(
this.languageResolver.language,
formatOptionsNoUnit
this.step
? {
...formatOptionsNoUnit,
maximumFractionDigits: this.digitsAfterDecimal,
}
: {
...formatOptionsNoUnit,
}
);
try {
this._numberFormatter = new NumberFormatter(
this.languageResolver.language,
this.formatOptions
this.step
? {
...this.formatOptions,
maximumFractionDigits: this.digitsAfterDecimal,
}
: {
...this.formatOptions,
}
);
this._forcedUnit = '';
this._numberFormatter.format(1);
Expand All @@ -554,6 +580,7 @@ export class NumberField extends TextfieldBase {
}
this._numberFormatter = this._numberFormatterFocused;
}
this._lastDigitsAfterDecimal = this.digitsAfterDecimal;
}
return this.focused
? this._numberFormatterFocused
Expand All @@ -562,6 +589,8 @@ export class NumberField extends TextfieldBase {

private _numberFormatter?: NumberFormatter;
private _numberFormatterFocused?: NumberFormatter;
private _lastDigitsAfterDecimal: number = 0;
private digitsAfterDecimal: number = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep this on the class, or can we use cache clearing to trigger the use of valueFormatter as a getter and contain this value therein?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can totally do that


protected get numberParser(): NumberParser {
if (!this._numberParser || !this._numberParserFocused) {
Expand Down
21 changes: 21 additions & 0 deletions packages/number-field/test/inputs.test.ts
Expand Up @@ -314,6 +314,27 @@ describe('NumberField - inputs', () => {
expect(el.value).to.equal(255);
});
});
describe('with floating point numbers', () => {
it('do not crash the Number Field', async () => {
const el = await getElFrom(minMax(minMax.args));
el.setAttribute('min', '0.1');
el.setAttribute('step', '0.01');
el.setAttribute('value', '0.5');
await elementUpdated(el);

el.focus();
await sendKeys({
type: '6',
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
});
await elementUpdated(el);
expect(el.formattedValue).to.equal('0.56');
await sendKeys({
press: 'Enter',
});
await elementUpdated(el);
expect(el.value).to.equal(0.56);
});
});
describe('locale specific', () => {
it('can determine the group symbol', async () => {
const [languageContext] = createLanguageContext('es-ES');
Expand Down
26 changes: 26 additions & 0 deletions packages/slider/stories/slider.stories.ts
Expand Up @@ -450,6 +450,32 @@ editableWithDefaultValue.swc_vrt = {
skip: true,
};

export const editableWithFractionValue = (
args: StoryArgs = {}
): TemplateResult => {
return html`
<div style="width: 500px; margin: 12px 20px;">
<sp-slider
editable
max="255"
min="0.1"
value="0.5"
step="0.01"
default-value="18"
@input=${handleEvent(args)}
@change=${handleEvent(args)}
...=${spreadProps(args)}
>
Angle
</sp-slider>
</div>
`;
};

editableWithFractionValue.swc_vrt = {
skip: true,
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
};

export const editableDisabled = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin: 12px 20px;">
Expand Down