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

numericFormatter locale issue #1503

Merged
merged 21 commits into from
Sep 30, 2024

Conversation

RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst commented Sep 26, 2024

Description

The numeric formatter fails on slider element input elements for a user with a locale that uses comma decimals, eg. DE 🇩🇪

The comma seperator isn't displayed and the return is an integer instead of a float.

GitHub Issue

#1499

Type of Change

  • ✅ Bug fix (non-breaking change which fixes an issue)
  • ✅ Testing

Testing Checklist

  • ✅ Existing Tests still pass
  • ✅ New Tests Added

@RobAndrewHurst RobAndrewHurst changed the title numericFormatter test numericFormatter locale issue Sep 26, 2024
@RobAndrewHurst RobAndrewHurst added Bug A genuine bug. There must be some form of error exception to work with. Testing Changes relating to existing or new unit tests. labels Sep 26, 2024
@RobAndrewHurst RobAndrewHurst marked this pull request as draft September 26, 2024 11:56
@RobAndrewHurst
Copy link
Contributor Author

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat

I think this is what we are looking for to solve this.

@RobAndrewHurst
Copy link
Contributor Author

@RobAndrewHurst
Copy link
Contributor Author

I have added in a number formatter on both the formatNumericValue & unformatStringValue functions. The formatter is an object that is created from a given locale. This is how we determine how numbers are formatted.

// Create a number formatter using the specified locale
const numberFormatter = new Intl.NumberFormat(params.formatterParams.locale);

I then use the formatter to break down the number into parts (digits, decimal seprator etc.)

const parts = numberFormatter.formatToParts(1.1);

After that I find the decimal separator which is used in the locale, by looking for the 'decimal' type in the parts array.

// Find the decimal separator used in this locale
const decimalSeperator = parts.find(part => part.type === 'decimal')?.value;

Then we replace the locale-specific decimal separator in the stringValue with a period. This normalises the number so that Javascript can perform a parseFloat.

// Replace the locale-specific decimal separator with a standard period
const normalisedValue = stringValue.replace(decimalSeperator, '.');

Then we replace any characters that aren't digits, periods, or minus signs. This will clean up thousand separators.

// Remove any non-numeric characters, except for period and minus sign
const cleanedValue = normalisedValue.replace(/[^\d.-]/g, '');

@RobAndrewHurst
Copy link
Contributor Author

Changes to how sliders get formatted as changed.

The value coming from a slider to the numericInput function input will not be formatted unlike a value from a numericInput. Therefore we need a check if we need to format that value with the utility.

I have added in a flag onRangInput from the slider onRangeInput function to indicate to the input function that we do not need to format the value.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

I pushed one commit, the oninput was not calling onRangeInput correctly. I have updated it to this line of code:

oninput=${e => onRangeInput(e, params)}>`

I have tested this using English, German, French and Thai in XYZ and all working for me :)

Copy link
Contributor

@AlexanderGeere AlexanderGeere left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

On an editable infoj entry, with a value of say 9999 - it is impossible to set this back to null. this should be possible.

This fails as CSS invalid gets applied so you can't save it.

When using a range slider on the infoj entry too, if the value is null - then the slider does not appear, which it should.

@RobAndrewHurst
Copy link
Contributor Author

When using a range slider on the infoj entry too, if the value is null - then the slider does not appear, which it should.

image

This is because of this logic

Copy link
Member

@cityremade cityremade left a comment

Choose a reason for hiding this comment

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

looks good to me. Icon scaling with default set with 2 decimals reformats ok. Also when editing numeric location fields incorrect separator is prevented 👍

Copy link

sonarcloud bot commented Sep 30, 2024

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

All good with me now - thanks @RobAndrewHurst

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

lgtm

@RobAndrewHurst RobAndrewHurst merged commit 1de8d2a into GEOLYTIX:main Sep 30, 2024
5 checks passed
@RobAndrewHurst RobAndrewHurst deleted the numeric_formatter_locale branch September 30, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with. Testing Changes relating to existing or new unit tests. v4.x.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comma Decimal in Numeric Formatter for Slider Element
5 participants