Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
290 stars 75 forks source link

Epic: move localizing numbers using numberingSystem to render block #6399

Closed anveshmekala closed 1 year ago

anveshmekala commented 1 year ago

Actual Behavior

While localizing numbers for a specific numberingSystem, it is cached by the browser and used by other components in the same page.

Expected Behavior

localized numbers using numberingSystem should be specific for a component and should not be applied to other components in the same page.

Reproduction Sample

https://codepen.io/ANVESHMEKALA/pen/YzjaveB?editors=100

Reproduction Steps

1.Click on the stepper item with arabic numbers 2.Then, click on the stepper item above with latin numbers 3.Observe that latin numbers changes to arabic.

Reproduction Version

beta.97

Relevant Info

List of affected components:

Please feel free to add if any other components experience similar behavior.

Regression?

No response

Impact

No response

Esri team

N/A

benelan commented 1 year ago

While localizing numbers for a specific numberingSystem, it is cached by the browser and used by other components in the same page. localized numbers using numberingSystem should be specific for a component and should not be applied to other components in the same page.

Small clarification in case someone grabs this before I do - we want the components to continue using the same NumberFormatter if they have matching lang/numberingSystem (I assume this is the case in 99% of apps).

Adding it to the render block just makes sure the component is always using a NumberFormatter for its lang/numberingSystem if the app has components with different i18n.

benelan commented 1 year ago

@anveshmekala numberFormatOptions is already set in calcite-input-date-picker's render block:

https://github.com/Esri/calcite-design-system/blob/4b4c43c952b54a6a74d41e4257607ad98bc2efe4/packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx#L505-L511

When you created the checklist above, did you go through and check all the components' code, or was it based on some behavior you saw? Let me know if there is more investigation needed on my end. Thanks!

anveshmekala commented 1 year ago

i remember going through each component. I would recommend more investigation just to make sure no other components had implemented this since the creation of the epic.

benelan commented 1 year ago

Looked into this a bit more and numberStringFormatter.numberFormatOptions only needs to be called in the render block if a number is being formatted in the render block via numberStringFormatter.localize().

Some components like calcite-input store the localized number as State instead. This works because calcite-input rerenders won't cause the value to be relocalized so there won't be a conflict, even if a different component changes the lang of the formatter cached by NumberStringFormat.

Instead, calcite-input sets numberStringFormatter.numberFormatOptions in event handlers and other places were the value can change before it localizes the number.

I can try to give a more detailed explanation if that was confusing.

It could also be worth moving the numberStringFormatter.localize() calls out of the render blocks when possible to improve performance.

github-actions[bot] commented 1 year ago

Installed and assigned for verification.

benelan commented 1 year ago

Created #7920 based on my comment above.

geospatialem commented 1 year ago

Closing per https://github.com/Esri/calcite-design-system/issues/6399#issuecomment-1743803243, also see https://github.com/Esri/calcite-design-system/issues/6399#issuecomment-1741608262 for details.