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

STCOM-1150: Remove unchecked calls to Moment.locale #2025

Open
wants to merge 7 commits into
base: b10.3
Choose a base branch
from

Conversation

ncovercash
Copy link
Member

@ncovercash ncovercash commented Apr 14, 2023

Jira STCOM-1150

This fixes the bug reported in STCOM-1150 and causing UICAL-267 by removing the erroneous call to moment.locale.

To summarize this Slack thread, the call to moment.locale for locales unknown to Moment (such as en-SE for Swedish English) caused Moment to fallback to default locale settings, however, it would also cache these default settings as the correct locale for en-SE. When Timepicker later checked to see if Moment knew the locale, Moment then reported that it did, returning incorrectly assumed locale information, resulting in 24-hour Timepickers switching to 12-hour at reconstruction.

@JohnC-80 / @zburke one of y'all will likely want to clone this branch over to folio-org to check CI (I don't have permissions here), however, running tests revealed no new failures (the only date-related failure was due to the change of the space in locale times during a recent Chrome update (replaced with \u202f)). Fixed that here too, for sanity's sake.

@github-actions
Copy link

github-actions bot commented Apr 14, 2023

Jest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit 093f62c. ± Comparison against base commit ea50095.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 14, 2023

BigTest Unit Test Statistics

       1 files  ±0         1 suites  ±0   11s ⏱️ ±0s
1 304 tests ±0  1 298 ✔️ ±0  6 💤 ±0  0 ±0 
1 307 runs  ±0  1 301 ✔️ ±0  6 💤 ±0  0 ±0 

Results for commit 093f62c. ± Comparison against base commit ea50095.

♻️ This comment has been updated with latest results.

@JohnC-80 JohnC-80 self-requested a review April 14, 2023 13:04
@zburke
Copy link
Member

zburke commented Apr 14, 2023

Reference for those \u202f changes. So much fun. 🙄

@ncovercash
Copy link
Member Author

Reference for those \u202f changes. So much fun. 🙄

Every time I deal with weird time zone/localization bugs, I am reminded of this amazing video

@ncovercash ncovercash marked this pull request as draft April 15, 2023 10:53
@zburke
Copy link
Member

zburke commented Apr 15, 2023

Ha! Great video, and he sure gets the moral correct (make somebody else do this if possible)! It's no small wonder that date/time pickers seem to always be the missing components in every single OSS component library!

I remember learning about unix in college and being totally bowled over by the command cal 9 1752. Part of it was the specific programming challenges, but more it was the eye-opening experience that something I'd thought was simple was actually really REALLY complicated, interesting, contentious, that a "factual" thing like the date could even be contentious, etc.

@ncovercash ncovercash marked this pull request as ready for review April 17, 2023 04:05
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

I believe moment is case-senstive WRT locales so those tests need to check for the exact same locale provided as the prop.

lib/Datepicker/tests/Datepicker-test.js Show resolved Hide resolved
util/dateTimeUtils.js Outdated Show resolved Hide resolved
lib/Timepicker/tests/Timepicker-test.js Show resolved Hide resolved
@ncovercash ncovercash requested a review from zburke April 19, 2023 17:07
@ncovercash ncovercash changed the title STCOM-1150: Remove unchecked call to Moment.locale STCOM-1150: Remove unchecked calls to Moment.locale Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants