Esri / calcite-design-system

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

[Date picker] Year field conflicts when used in coordination with the Alert component #6781

Closed geospatialem closed 10 months ago

geospatialem commented 1 year ago

Actual Behavior

Additional characters + and , (e.g., +2,022) are added when changing the year in the date-picker component when an alert component is used on the same app/page.

image

The behavior occurs when the alert and date-picker components are used together, and is present in 1.0.x and regardless of the alert component duration (no duration or a set timer).

datepicker-alert-year-bug

Expected Behavior

No additional characters should be added to the year (e.g., 2022).

Reproduction Sample

https://codepen.io/geospatialem/pen/mdzVjNj

Reproduction Steps

  1. Open the Codepen sample
  2. Observe the alert is present
  3. Open the "Information" action and change the date-picker's year value
  4. Observe the year adds a + character to the start and a , after the first numerical value.

Reproduction Version

1.2.0

Relevant Info

Identified via Community.

Regression?

No response

Priority impact

p3 - want for upcoming milestone

Impact

Moderate - occurs when both alert and date-picker are used together, but should be mitigated.

Esri team

Calcite (dev)

macandcheese commented 1 year ago

I can’t reproduce this 🤔.

Is there an OS / Browser reported to check or a method of changing the year that causes this (direct entry / selecting date cell)?

geospatialem commented 1 year ago

Is there an OS / Browser reported to check or a method of changing the year that causes this (direct entry / selecting date cell)?

Added a .gif in for the workflow, but was able to repro on Windows with Chrome, FF, Edge and Mac with Safari, Chrome, and FF.

jcfranco commented 1 year ago

This may be related to the numbering system util. cc @benelan

geospatialem commented 1 year ago

Was unable to repro with date-picker and stepper when stepper has numbered and/or numbering-system defined.

geospatialem commented 1 year ago

Additional research needed to determine the next steps as to where the conflict could be originating from with date-picker and alert when used together.

jcfranco commented 1 year ago

Found the culprit. alert, along with other components, sets its numberStringFormatter options within render(). date-picker only updates it when the locale changes.

Other components that might show the same issue are:

Related to https://github.com/Esri/calcite-design-system/issues/6399.

benelan commented 1 year ago

Yeah it looks like we will need to make sure the formatter is correct on every render if we want to continue caching it globally. The other option is caching the formatter per component so there is no way they conflict with eachother. I created an example of that set up a while ago:

https://github.com/Esri/calcite-design-system/compare/main...benelan/cache-formatter-per-component

I'd have to look at it again if you have any questions though lol. But caching per component would prevent checking the options on every render, which involves serializing objects:

https://github.com/Esri/calcite-design-system/blob/465e7601c9ccc1749260525a7563af8cbf370632/packages/calcite-components/src/utils/locale.ts#L385

There are usually only 2-3 properties on the object though, so it isn't super intensive. Any preferences @jcfranco? When you said we could cache the formatter I should have checked which way you meant.

Edit:

Woops, sorry I missed the mention a few months ago. I just cleared out my github notification inbox and I'm going to start checking that, because stuff gets buried in my work email.

jcfranco commented 1 year ago

Any preferences @jcfranco?

For this particular issue, we can proceed with arranging the formatter to be configured correctly per each render cycle.

For the caching matter, what you have in your branch seems promising. We should also look into how many unique formatters we would end up with if we wanted to cache similar to how other components cache date formatters where the (cached) formatter is a state prop and updated when a relevant prop is modified (e.g., https://github.com/Esri/calcite-design-system/blob/main/packages/calcite-components/src/components/date-picker/date-picker.tsx#L365).

geospatialem commented 10 months ago

Verified in 2.2.0-next.11 with https://codepen.io/geospatialem/pen/mdzVjNj.

Likely mitigated as part of https://github.com/Esri/calcite-design-system/pull/8518. :trophy: