Add change presentation hints to Forms to allow pop #2412
Conversation
| using System; | ||
| using MvvmCross.Core.ViewModels; | ||
|
|
||
| namespace MvvmCross.Forms.Views.Hints |
There was a problem hiding this comment.
If the hint is located in this namespace, and a user has defined all ViewModels in a separate dll from the views, then the hint won't be available.
I guess this kind of makes sense because the hint uses the View type, but then again... I'm not sure if it's a good practise to request presentation changes at a platform level.
Maybe the hint can take a ViewModel type?
There was a problem hiding this comment.
I agree with Nico here. Hints should probably be platform agnostic so they can be used anywhere no matter whether you use forms or not.
| using System; | ||
| using MvvmCross.Core.ViewModels; | ||
|
|
||
| namespace MvvmCross.Forms.Views.Hints |
There was a problem hiding this comment.
Same about this presentation hint
martijn00
force-pushed
the
forms-hints
branch
from
610693d
to
214df45
Compare
|
@Cheesebaron @nmilcoff Can you do another review? |
| Close((hint as MvxClosePresentationHint).ViewModelToClose); | ||
| var host = GetHostPageOfType<NavigationPage>(); | ||
| var page = host.Navigation.NavigationStack.OfType<IMvxPage>().FirstOrDefault(x => x.ViewModel.GetType() == removeHint.ViewModelToRemove) as Page; | ||
| host.Navigation.RemovePage(page); |
There was a problem hiding this comment.
How does RemovePage handle cases when page == null?
| { | ||
| Close((hint as MvxClosePresentationHint).ViewModelToClose); | ||
| var host = GetHostPageOfType<NavigationPage>(); | ||
| var page = host.Navigation.NavigationStack.OfType<IMvxPage>().FirstOrDefault(x => x.ViewModel.GetType() == removeHint.ViewModelToRemove) as Page; |
There was a problem hiding this comment.
This chain is awfully long. Also x is not very descriptive.
| if (HandlePresentationChange(hint)) return; | ||
|
|
||
| if (hint is MvxClosePresentationHint) | ||
| if (hint is MvxPopToRootPresentationHint popToRootHint) |
There was a problem hiding this comment.
GetHostPageOfType<NavigationPage>() is used in all the if statements. Can you just call this once and reuse?
| if (page is IMvxPage mvxPage && mvxPage.ViewModel.GetType() == popHint.ViewModelToPopTo) | ||
| { | ||
| page.Navigation.PopAsync(popHint.Animated); | ||
| break; |
| page.Navigation.PopAsync(popHint.Animated); | ||
| break; | ||
| } | ||
| else |
There was a problem hiding this comment.
remove redundant else as there is a break; above
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Feature
🆕 What is the new behavior (if this is a feature change)?
You can pop and pop to root in Forms
💥 Does this PR introduce a breaking change?
No
🐛 Recommendations for testing
Playground app
📝 Links to relevant issues/docs
🤔 Checklist before submitting