search

Esri / calcite-design-system

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

[Input Time Zone] Offset of `-660` is ignored #7957

Closed geospatialem closed 11 months ago

geospatialem commented 12 months ago

Check existing issues

Actual Behavior

  1. The input-time-zone component no longer supports setting value as -660 (as of 1.9.0). In these cases the user's default time zone is returned to the component.
  2. The UTC-12/GMT-12 doesn't have an associated location, and as a result is reverting to the user's time zone.

cc @JonUihlein @hgonzago

Expected Behavior

The component's value is supported across all time zones.

Reproduction Sample

https://codepen.io/jonuihlein/pen/OJraWgM

Reproduction Steps

  1. Open the sample
  2. Observe the value at -600 shows the expected value for GMT-10
  3. Change the value to any value higher than -600, such as -660 or -720
  4. The change should bring up GMT-11 and GMT-12 respectively, but it does not
    • Instead, it updates to the user's time zone

Reproduction Version

1.9.0

Relevant Info

No response

Regression?

1.8.0

Priority impact

p1 - need for current milestone

Impact

Very high - impacts Online and Maps SDK for JS teams in the October releases.

Calcite package

Esri team

ArcGIS Maps SDK for JavaScript

jcfranco commented 12 months ago

Pinpointed the issue and it's with timezone-groups. It's not creating the proper group for time zones that have an offset of -11. I'll take a closer look.

jcfranco commented 12 months ago

This should be addressed by https://github.com/Esri/calcite-design-system/pull/7962.

jcfranco commented 11 months ago

The above PR landed. @geospatialem Can you test once there's an update to next? 🙇

geospatialem commented 11 months ago

The above PR landed. @geospatialem Can you test once there's an update to next? 🙇

This LGTM. 🌟

cc @JonUihlein @hgonzago for additional testing, here's a Codepen with next and the value set to -660.

github-actions[bot] commented 11 months ago

Installed and assigned for verification.

github-actions[bot] commented 11 months ago

Installed and assigned for verification.

geospatialem commented 11 months ago

Verified in https://codepen.io/geospatialem/pen/MWZLMLp with 1.9.2-next.3, where GMT-12 displays without a grouping:

image

jcfranco commented 11 months ago

@geospatialem Your screenshot reminded me that for GMT-12 specifically, the label might be confusing to users. It represents Etc/GMT+12 which intentionally uses the inverted sign, but its offset is indeed -12. This is the only group where there is no other IANA time zone identifier with a region. We could tackle this in a separate issue if there's a clearer way to present this info. cc @ashetland

ashetland commented 11 months ago

@geospatialem Your screenshot reminded me that for GMT-12 specifically, the label might be confusing to users. It represents Etc/GMT+12 which intentionally uses the inverted sign, but its offset is indeed -12. This is the only group where there is no other IANA time zone identifier with a region. We could tackle this in a separate issue if there's a clearer way to present this info. cc @ashetland

We could look at possible improvements to the name mode as well.

ashetland commented 11 months ago

@geospatialem Your screenshot reminded me that for GMT-12 specifically, the label might be confusing to users. It represents Etc/GMT+12 which intentionally uses the inverted sign, but its offset is indeed -12. This is the only group where there is no other IANA time zone identifier with a region. We could tackle this in a separate issue if there's a clearer way to present this info. cc @ashetland

We could look at possible improvements to the name mode as well.