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): updated number field to respect all locales #4508

Merged
merged 5 commits into from
May 28, 2024

Conversation

blunteshwar
Copy link
Collaborator

@blunteshwar blunteshwar commented May 27, 2024

Description

Number-field was not respecting other locales than 'en'.

Related issue(s)

Motivation and context

The function convertValueToNumber was not receiving correct input when locale was anything other than 'en'.
In function stepBy earlier we were doing this.inputElement.value = value.toString(); which was not respecting locales.
Thus to respect locales we are now formating the value like this this.inputElement.value = this.numberFormatter.format(value);

How has this been tested?

I wrote some tests in Number-field.test.ts
Test here

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@blunteshwar blunteshwar requested a review from a team May 27, 2024 12:13
Copy link

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Copy link

github-actions bot commented May 27, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.99 0.99
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 225.537 kB 210.607 kB 210.275 kB 🏆
Scripts 54.654 kB 48.292 kB 47.999 kB 🏆
Stylesheet 35.059 kB 30.403 kB 30.397 kB 🏆
Document 5.998 kB 5.271 kB 🏆 5.273 kB
Font 126.871 kB 126.641 kB 126.606 kB 🏆

Request Count

Category Latest Main Branch
Total 48 45 45
Scripts 37 37 37
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

Copy link

github-actions bot commented May 27, 2024

Tachometer results

Chrome

number-field permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 523 kB 65.78ms - 67.92ms - faster ✔
9% - 12%
6.59ms - 9.29ms
branch 511 kB 73.96ms - 75.61ms slower ❌
10% - 14%
6.59ms - 9.29ms
-

slider permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 74.70ms - 76.31ms - faster ✔
2% - 5%
1.23ms - 3.77ms
branch 467 kB 77.02ms - 78.98ms slower ❌
2% - 5%
1.23ms - 3.77ms
-
Firefox

number-field permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 523 kB 149.05ms - 154.95ms - faster ✔
6% - 11%
9.54ms - 17.74ms
branch 511 kB 162.80ms - 168.48ms slower ❌
6% - 12%
9.54ms - 17.74ms
-

slider permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 163.94ms - 170.50ms - faster ✔
2% - 8%
3.02ms - 14.22ms
branch 467 kB 171.29ms - 180.39ms slower ❌
2% - 9%
3.02ms - 14.22ms
-

@blunteshwar blunteshwar removed the request for review from a team May 27, 2024 12:41
@TarunAdobe TarunAdobe changed the title Bug/number field not respecting locale fix(number-field): updated number field to respect all locales May 27, 2024
@TarunAdobe TarunAdobe requested a review from a team May 27, 2024 14:07
Copy link
Contributor

@TarunAdobe TarunAdobe left a comment

Choose a reason for hiding this comment

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

This works!!! 🍡

@Rajdeepc Rajdeepc added bug Something isn't working Component: Number Field labels May 28, 2024
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

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

@blunteshwar Can you also create a story for this so that it surfaces up to the VRT's and we can make sure it always passes for other locales?

@blunteshwar blunteshwar merged commit cc6e928 into main May 28, 2024
58 checks passed
@blunteshwar blunteshwar deleted the bug/number-field-not-respecting-locale branch May 28, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Component: Number Field
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Slider and Numberfield don't respect different locale with fractional steps
3 participants