-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix korean cupertino datepicker datetime order #163850
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
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@chul0061 Thanks for the contribution! Can you add a test for this change? |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM. Just needs a small test in packages/flutter_localizations/test/cupertino/translations_test.dart.
|
@MitchellGoodwin |
|
Looks like the analyzer is complaining about formatting in the test file. You can fix that by running one of the commands mentioned with the error here: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8721874116024728257/+/u/run_test.dart_for_analyze_shard_and_subshard_None/stdout Or by using the dart formatter, which we recently enabled. |
|
@MitchellGoodwin fixed the issue. |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for the fix. This will need a secondary reviewer to get merged, so I'll ask around.
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thank you for correcting this!
@jayoung-lee Could you confirm that the correct time format in Korean is AM/PM : hour : minute?

Currently, in the Korean locale (
ko), theCupertinoDatePickerdisplays the time in the order ofhour : minute : AM/PM.However, the correct format for Korean conventions is
AM/PM : hour : minute.This PR modifies the
CupertinoDatePickerto display the time in the correct order when the Korean locale is used.Changes
ko) locale inCupertinoDatePicker.hour : minute : AM/PMAM/PM : hour : minutePre-launch Checklist
///).