-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
UWP DI updates #1685
UWP DI updates #1685
Conversation
Known Issue:
|
{ | ||
if (args.StartKind == StartKinds.Launch) | ||
{ | ||
NavigationService.NavigateAsync(nameof(MainPage)); | ||
NavigationService.NavigateAsync("/MainPage"); |
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.
can't we use $"/{nameof(MainPage)}"? I don't like suggesting magic strings
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.
two things here... 1) using nameof
on a View type is actually worse IMO... 2) remember this is a Sandbox app
This error on launch is because of UWP being forced on 6.2.2 by MSBuild.Sdk.Extras. Created PR to roll back to 6.1.9: novotnyllc/MSBuildSdkExtras#152 |
…6.2.X being unlisted from NuGet
downgrading to 6.1.9 per @bartlannoeye's suggestion, I noticed we were targeting different versions of the UWP package between Prism and the Sandbox app... now that these are all the same the Sandbox app actually does work. |
|
||
namespace Prism.Navigation | ||
{ | ||
public static class NavigationServiceRegistry |
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 feel like this should be a NavigationServiceFactory.
// required for view-models | ||
|
||
containerRegistry.Register<INavigationService, NavigationService>(NavigationServiceParameterName); | ||
containerRegistry.Register<IPlatformNavigationService, NavigationService>(NavigationServiceParameterName); |
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.
why do we have a secondary registration for IPLatformNavigationService? This should not be needed.
|
||
namespace Prism.Services | ||
{ | ||
public class GestureService : IGestureService | ||
public class GestureService : IGestureService, IDestructibleGestureService |
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.
Why have a new interface for Destroy that is specific to the gesture service? Just bake it part of the IGestureServcie
@dansiegel , @brianlagunas In the old way, the implementation is like this: |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description of Change
Updates Gesture Service and NavigationService to ensure that these service instances are constructed by the DI container. Refactors the naming of some of the internal use interfaces to better align with Prism conventions.
API Changes
List all API changes here (or just put None), example:
Added:
Changed:
Removed:
Methods from IPrismApplicationBase were made protected rather than public
Behavioral Changes
Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.
PR Checklist