-
Notifications
You must be signed in to change notification settings - Fork 633
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
Add support for dateStyle and timeStyle for Android #926
Comments
@mganandraj we did some local testing with your PR. |
Hi @dlebedynskyi, |
Summary: This PR is adding implementation for dateStyle and timeStyle options to Intl:DateTimeFormat service for Android API 24+. Issue No : #926 This PR is adding implementation for dateStyle and timeStyle options to Intl:DateTimeFormat service. Pull Request resolved: #927 Test Plan: test262 based test suite is passing. Reviewed By: mattbfb Differential Revision: D45185307 Pulled By: jpporto fbshipit-source-id: 611f16080eb2e652606d95e0502592f56970dd80
@mganandraj sorry for late reply. We think there is a bug in how it is implemented. The way it is done, it does not match the spec - https://tc39.es/ecma402/#datetimeformat-objects.
as example
this should just be "W" since using "weekday" negates the need for defaults. Fix that we tested was here in https://github.com/facebook/hermes/blob/main/lib/VM/JSLib/Intl.cpp#L272 - {u"weekday", platform_intl::Option::Kind::String, 0},
+ {u"weekday", platform_intl::Option::Kind::String, kDateRequired}, We had tested your change prior it was merged, and took time to find fix. |
Hello @dlebedynskyi .. 100% agreed. This bug has existed from day 1 and somehow left unnoticed. We do run most relevant Test262 test cases which doesn't seem to cover this. I'd appreciate if you can send a PR. |
Summary: Original Author: anandrag@microsoft.com Original Git: a72115f This PR is adding implementation for dateStyle and timeStyle options to Intl:DateTimeFormat service for Android API 24+. Issue No : #926 This PR is adding implementation for dateStyle and timeStyle options to Intl:DateTimeFormat service. Original Reviewed By: mattbfb Original Revision: D45185307 Pull Request resolved: #927 Reviewed By: neildhar Differential Revision: D47690090 fbshipit-source-id: f43e36a6c16f092ec708bc69a4be34075f82ae4a
Hey all, would you recommend that we use #927 as a patch in our app? Or is there likelihood of this getting merged soon? |
Hey @nandorojo, it looks like this was already merged in a72115f, and is present in RN 0.73. Closing this issue. |
Problem
Intl on Android is still missing dateStyle and timeStyle. iOS already has it since v0.12.
Solution
Ask is to add support for missing Intl.DateTimeFormat API for Android dateStyle/timeStyle.
Additional Context
If possible, would be great to add rest of missing API
to close rest of missing options on Intl.DateTimeFormat as polyfill for Intl.DateTimeFormat is complex and heavy, and is currently required when dateStyle/timeStyle are used.
Related #23 (comment)
The text was updated successfully, but these errors were encountered: