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

feat: add pt-BR hour format #984

Conversation

CarolineLCa
Copy link
Contributor

Summary of Changes

Adding pt-BR format hour (24h) when this language is selected.

Related Issues

N/A

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this);
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components;
  • Updated documentation as needed for changed code and new or modified features;
  • Added sufficient tests so that overall code coverage is not reduced.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated tests have passed.

PR template adapted from the Python attrs project.

Signed-off-by: Caroline Lucas Calheirani <[email protected]>
@CarolineLCa CarolineLCa requested a review from a team as a code owner October 2, 2023 17:38
@CarolineLCa CarolineLCa changed the title add pt-BR hour format feat:add pt-BR hour format Oct 2, 2023
@CarolineLCa CarolineLCa changed the title feat:add pt-BR hour format feat: add pt-BR hour format Oct 2, 2023
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #984 (6190728) into main (1ca8bbd) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #984      +/-   ##
==========================================
+ Coverage   59.24%   59.26%   +0.01%     
==========================================
  Files         176      176              
  Lines        5084     5086       +2     
  Branches     1451     1452       +1     
==========================================
+ Hits         3012     3014       +2     
  Misses       2046     2046              
  Partials       26       26              
Files Coverage Δ
packages/legacy/core/App/utils/helpers.ts 51.63% <66.66%> (+0.28%) ⬆️

@bryce-mcmath bryce-mcmath merged commit 05fbe8b into openwallet-foundation:main Oct 3, 2023
5 checks passed
@thiagoromanos
Copy link
Contributor

thiagoromanos commented Oct 3, 2023

Wouldn't it be better if this format be a i18n key? Many apps that support pt-BR uses 12 hour format, so 24 hours is not a strict rule. Checking device's configuration would be nice (https://github.com/steffenagger/react-native-device-time-format)

@bryce-mcmath
Copy link
Contributor

@CarolineLCa can you implement the above as an alternative to this change?

@CarolineLCa
Copy link
Contributor Author

@CarolineLCa can you implement the above as an alternative to this change?

@thiagoromanos @bryce-mcmath

I have checked this dependence and tried to implement it, but DeviceTimeFormat.is24HourFormat() is an async function while our formatTime() is non-async so this change would not only affect this function but also a lot of other components that use it.

Captura de Tela 2023-10-04 às 06 39 20

@thiagoromanos
Copy link
Contributor

thiagoromanos commented Oct 4, 2023

@CarolineLCa can you implement the above as an alternative to this change?

@thiagoromanos @bryce-mcmath

I have checked this dependence and tried to implement it, but DeviceTimeFormat.is24HourFormat() is an async function while our formatTime() is non-async so this change would not only affect this function but also a lot of other components that use it.

Captura de Tela 2023-10-04 às 06 39 20

Gonna open an issue for that. It'll need a lot of refactoring indeed, but it would be great to have the time format matching the device's format.

@thiagoromanos
Copy link
Contributor

Opened the issue #986 for that.

@CarolineLCa CarolineLCa deleted the feat/add-pt-br-hour-format branch October 25, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants