Auto-detect RTL locale and apply to root view #4732
Conversation
| } | ||
|
|
||
| void I18nHelper::setAllowRTL(bool allowRTL) { | ||
| m_allowRTL = allowRTL; |
There was a problem hiding this comment.
Are we okay with this as a global setting instead of per instance? Wondering why we moved to a singleton. #Resolved
There was a problem hiding this comment.
The singleton is there for simplicity and is modeled after how it was done on android. NativeUIManager now depends on functionality inside I18nManager. With the singleton helper we don't need to share the entire I18nModule with the UIManagerModule, which would lead to awkward lifetime issues (the modules are created lazily).
From a high level as to if it's OK to use a singleton, I think so. The information being queried from the OS is per application instance anyway, and it seems not useful for different instances to set allowRTL/forceRTL differently for different instances so simpler seems better to me.
| // Push the appropriate FlowDirection into the root view. | ||
| auto flow = I18nHelper::Instance().getIsRTL() ? winrt::FlowDirection::RightToLeft : winrt::FlowDirection::LeftToRight; | ||
| view.as<winrt::FrameworkElement>().FlowDirection( | ||
| I18nHelper::Instance().getIsRTL() ? winrt::FlowDirection::RightToLeft : winrt::FlowDirection::LeftToRight); |
There was a problem hiding this comment.
can you please use the updated namespaces? (xaml::FrameworkElement, etc.) #Resolved
There was a problem hiding this comment.
ghost
added
the
Needs: Author Feedback
ghost
removed
the
Needs: Author Feedback
|
|
||
| class I18nHelper { | ||
| public: | ||
| static I18nHelper &Instance(); |
There was a problem hiding this comment.
) [](start = 30, length = 1)
Please consider to add noexcept to methods and constructors to keep binary code smaller.
vnext/ReactUWP/Modules/I18nModule.h
Outdated
| I18nHelper(); | ||
|
|
||
| void setInfo(I18nModule::I18nInfo &&i18nInfo); | ||
| std::string getLocaleIdentifier(); |
There was a problem hiding this comment.
) [](start = 34, length = 1)
Can it be const? #Resolved
vnext/ReactUWP/Modules/I18nModule.h
Outdated
|
|
||
| private: | ||
| I18nModule::I18nInfo m_i18nInfo; | ||
| bool m_allowRTL = true; |
There was a problem hiding this comment.
= true [](start = 17, length = 7)
The latest 'fashion' is to use the universal initializers:
bool m_allowRTL{true};
E.g. the std::atomic fails to initialize otherwise in Clang. #Resolved
kmelmon
added
the
AutoMerge
|
Hello @kmelmon! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
4 participants
Fixes #4684
Fixes #4676
Fixes #4670
This change adds support for auto-detecting we're in an RTL locale, and flipping the FlowDirection on root view(s) to RightToLeft. It also adds support for two i18nManager APIs:
forceRTL: Used for testing, forces the framework into RTL mode even on LTR locales
allowRTL: When set true, the auto-detection logic runs and flips the root into RTL in RTL locales. The default value is true, same as the other platforms.
Note that if you toggle the primary language, you must restart the app for changes to be applied. This is consistent with other platforms.
Tested this change out in the RTLExample page in RNTester which has a control to toggle forceRTL.
Microsoft Reviewers: Open in CodeFlow