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

Localized date rendering #1013

Merged
merged 9 commits into from
Feb 3, 2022
Merged

Localized date rendering #1013

merged 9 commits into from
Feb 3, 2022

Conversation

vvikramraj
Copy link
Contributor

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #898

Description
Change to display dates in a locale-specific DateFormat.DEFAULT format.

Alternative(s) considered
NA

Type
Choose one: Bug fix

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct
  • I have read How to Contribute
  • I have read the Developer's guide
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
  • I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #1013 (2518757) into master (f0f2684) will increase coverage by 0.01%.
The diff coverage is 42.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1013      +/-   ##
============================================
+ Coverage     84.49%   84.51%   +0.01%     
  Complexity      608      608              
============================================
  Files           133      134       +1     
  Lines         10277    10275       -2     
  Branches        753      756       +3     
============================================
  Hits           8684     8684              
+ Misses         1218     1215       -3     
- Partials        375      376       +1     
Impacted Files Coverage Δ
...ws/QuestionnaireItemDatePickerViewHolderFactory.kt 36.50% <0.00%> (+0.27%) ⬆️
...ndroid/fhir/datacapture/utilities/MoreLocalDate.kt 75.00% <75.00%> (ø)
...va/com/google/android/fhir/db/impl/DatabaseImpl.kt 88.88% <0.00%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0f2684...2518757. Read the comment docs.

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

Thanks @vvikramraj! Great job writing your first PR in the repo :)

Left a few comments - also, can you please think about handing this for the date time picker? Either same PR or create a follow up PR/issue.

Copy link
Collaborator

@jingtang10 jingtang10 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 @vvikramraj - just one comment to update the import.

get() {
val date = Date.from(atStartOfDay(ZoneId.systemDefault())?.toInstant())
return if (isAndroidIcuSupported())
DateFormat.getDateInstance(DateFormat.DEFAULT).format(date)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be android.icu.text.DateFormat and not java.text.DateFormat

https://developer.android.com/guide/topics/resources/internationalization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-| Done.

Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

👍

@vvikramraj vvikramraj merged commit 7661938 into master Feb 3, 2022
@vvikramraj vvikramraj deleted the vr/date-format branch February 3, 2022 09:54
ktarasenko pushed a commit that referenced this pull request Feb 16, 2022
* Localized date rendering

* Addressing code review comments

* Switching between DateFormat and SimpleDateFormat based on SDK version

* Fixes from running spotless check and apply

* Fixing imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use ICU library to format date
2 participants