-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(Shared): Migrate react page and dependencies #846
feat(Shared): Migrate react page and dependencies #846
Conversation
I diverged the ReactPage file because of the differences in handling keyboard combos. |
@@ -44,7 +45,11 @@ public IReadOnlyList<INativeModule> CreateNativeModules(ReactContext reactContex | |||
viewManagerList, | |||
_uiImplementationProvider.Create( | |||
reactContext, | |||
viewManagerList)); | |||
viewManagerList) | |||
#if !WINDOWS_UWP |
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.
I wonder if a cleaner approach would be something like...
#if WINDOWS_UWP
var uiImplementation = _uiImplementationProvider.Create(
reactContext,
viewManagerList));
#else
var uiImplementation = _uiImplementationProvider.Create(
reactContext,
viewManagerList,
Application.Current.MainWindow));
#endif
uiManagerModule = new UIManagerModule(
reactContext,
viewManagerList,
uiImplementation));
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.
I know it's more code, but (personally) I think it's more readable and less likely to break / confuse someone if we have to add UIManagerModule parameters down the road (which I think is likely enough)
In reply to: 85830373 [](ancestors = 85830373)
That's fine. At some point down the road we could try and come up with an abstraction that managed the subscribing and unsubscribing to the keyboard events and then just diverge that abstraction. In reply to: 257414578 [](ancestors = 257414578) |
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 based on all feedback here and in Reactiflux being implemented.
* migrate ReactPage and dependencies to the Shared project
Replaces #842