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

Introduce a locale sensitive date-picker #746

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

samuelmale
Copy link
Member

@samuelmale samuelmale commented Aug 11, 2023

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This PR basically introduces a locale-sensitive date-picker to the style-guide. So far it's a mix of Carbon and Adobe React Spectrum's (ARS) date-picker.

How does it work?

ARS' date-picker has more concrete support for Internationalization out of the box. The fact that it supports non-gregorian calendars was the most compelling factor for choosing this library. We simply render ARS's date-picker if the current locale is supported by ARS, otherwise, we default to Carbon's date-picker. Take note that all the "English" specific locales have been excluded from the ARS' supported locales.

What next?

  • Carbonize the ARS's date-picker for more consistency
  • Get rid of Carbon's date-picker

Screenshots

Screenshot 2023-08-11 at 14 53 53
Screenshot 2023-08-11 at 14 55 36
Screenshot 2023-08-11 at 14 56 11
Screenshot 2023-08-11 at 14 56 42

Related Issue

N/A

Other

Resources

Shouts

Kudos to @sinteco for working on this

@samuelmale
Copy link
Member Author

Early comments:

React Spectrum's picker has a white (rgb(248, 248, 248)) background color which makes it look a little weird if the container doesn't explicitly define boundaries. Maybe we should use the same color as Carbon's?

@samuelmale samuelmale changed the title Introduce locale sensitive date-picker Introduce a locale sensitive date-picker Aug 11, 2023
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks for the nice work on this! A few things (this whole thing is a bit tricky), but hey.

@mseaton
Copy link
Member

mseaton commented Aug 16, 2023

I added @mogoodrich as a reviewer, as I know he is interested in this functionality.

@samuelmale samuelmale force-pushed the feat/locale-sensitive-datepicker branch from 2e6772d to 6953727 Compare August 17, 2023 19:49
Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

I'm not familiar with this widget, so didn't do a full review, but please see my comments @samuelmale @ibacher @mseaton , especially the one about setting UTC date.

@ibacher
Copy link
Member

ibacher commented Aug 22, 2023

Maybe we should use the same color as Carbon's?

I think this is the last thing for this particular PR. We essentially just style the react-spectrum component so it looks exactly like the Carbon component. Otherwise, it will clash with the rest of the UI.

Really nice work on this @samuelmale!

@ebambo
Copy link

ebambo commented Aug 31, 2023

@samuelmale thank you so much for working on this. @mseaton @ibacher @mogoodrich I agree and value everyone's comments and ideas shared for this PR but I have a special request: can we revert (temporarily) to the initial solution Samuel has proposed, of having the carbon date picker being returned if the timezone is English otherwise return the spectrum data picker. Am asking this because the Ethiopia Team needs this feature incorporated as soon as possible, they are rolling out O3 next week. They had initially forked everything but we managed to move them away from that. The plan is not to leave this as is but work on carbonizing the component as recommended.
CC @gracepotma

@mogoodrich
Copy link
Member

@ebambo I haven't been following this super closely, but I'm generally okay with doing something temporary in order for you to move forward, as long as there is a plan to move away from the temporary solution. Defer to @samuelmale and @ibacher on the specifics.

@ibacher
Copy link
Member

ibacher commented Aug 31, 2023

@ebambo In principle, I think it's fine to integrate something like the earlier version. The comments Mark and I made specifically around date calculations will need to be fixed. It would also be important to swap the call to getConfigStore() for a call to useConfig() because, as is, this is a memory leak. Finally, the last version of this PR that uses the Carbon component incorrectly assigns any value as a property of the DatePickerInput, instead of the DatePicker component.

As long as those minimal changes are addressed and we have some assurance that changing from DatePicker -> OpenmrsDatePicker will be tested with both the Ge'ez and Gregorian calendars, we can go ahead and merge the earlier version of this.

@ebambo
Copy link

ebambo commented Sep 1, 2023

@ibacher @mogoodrich thank you so much for prompt response. @ibacher sure thing, we will address the points you raised and merge the PR.

@samuelmale samuelmale force-pushed the feat/locale-sensitive-datepicker branch from 5b04af7 to c535313 Compare September 1, 2023 09:19
Comment on lines +1 to +5
export const supportedLocales = {
am: "Amharic",
am_ET: "Amharic (Ethiopia)",
ti_ET: "Tigrinya (Ethiopia)",
};
Copy link
Member Author

Choose a reason for hiding this comment

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

To minimize potential chaos, I have restricted this to locales specific to Ethiopia only.

@ebambo ebambo merged commit 6b62a36 into openmrs:main Sep 4, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants