Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Fix timepicker in Finnish locales #916

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

eyelidlessness
Copy link
Contributor

@eyelidlessness eyelidlessness commented Sep 26, 2022

Fixes enketo/enketo-express#453. Opening as draft for now until the following are complete (unless we determine not to do the remaining work):

  • Use the Intl.DateTimeFormat API rather than regex to determine time structure.
  • Use a locale-specific date string to determine whether a time string has a meridian component. This is necessary because you can't new Date('01:00 pm').
  • Detect locale changes and updates the locale state used for the above accordingly.
  • Detect locale change to update widget state.
  • [ ] Format widget display state with locale-specific punctuation rather than hard coding the separator as :

@lognaturel
Copy link
Contributor

Nice! Do you see a big advantage to doing the last two points in this PR vs treating them as separate issues?

Both seem nice to have but not critical to me so it all depends on whether you think they’re quick or not.

@eyelidlessness eyelidlessness marked this pull request as ready for review September 27, 2022 19:30
@eyelidlessness
Copy link
Contributor Author

It was pretty straightforward to handle the language change. I decided to hold off on the separator though because it occurs to me there's probably other issues with that as well. Anyway, I think this is now ready for review!

test/spec/types.spec.js Outdated Show resolved Hide resolved
Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'm just having trouble identifying a test that would have failed previously. Worth doing a sanity check with Express before merging?

Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Looks great. @eyelidlessness mentioned some risk around date/times coming in from new submissiosn vs. existing submissions vs. defaults and whether those are all localized at the same time. As far as I can tell, the interaction with all of those is unchanged from the original regex-based solution so it doesn't seem like there's a lot of risk there to me. Still, I've made a note for QA to do some additional verification once it's available in Express!

@lognaturel lognaturel merged commit e03818e into enketo:master Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants