Skip to content
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

RFC: Stack Routing API #20

Open
RLittlesII opened this issue Oct 12, 2018 · 10 comments
Open

RFC: Stack Routing API #20

RLittlesII opened this issue Oct 12, 2018 · 10 comments

Comments

@RLittlesII
Copy link
Member

Add Stack Routing API

Summary

This RFC is based on https://github.com/reactiveui/ReactiveUI/issues/1107

The current implementation of RoutingState exposes navigation as a set of commands on an ObservableCollection<IRoutableViewModel>

Xamarin.Forms has built in navigation based on a stack data structure.

public interface INavigation
{
  IReadOnlyList<Page> ModalStack { get; }

  IReadOnlyList<Page> NavigationStack { get; }

  void InsertPageBefore(Page page, Page before);

  Task<Page> PopAsync();

  Task<Page> PopAsync(bool animated);

  Task<Page> PopModalAsync();

  Task<Page> PopModalAsync(bool animated);

  Task PopToRootAsync();

  Task PopToRootAsync(bool animated);

  Task PushAsync(Page page);

  Task PushAsync(Page page, bool animated);

  Task PushModalAsync(Page page);

  Task PushModalAsync(Page page, bool animated);

  void RemovePage(Page page);
}

Motivation

The current RoutingState currently has no support for the following concerns:

  1. Animation
  2. Navigation and Modal stack
  3. Passing parameters on navigation

I am proposing we introduce a reactive abstraction base on Custom Routing in ReactiveUI. This would provide view model based navigation and better reflect stack navigation patterns.

public interface IRoutingStack
{
    IObservable<IImmutableList<IRoutableViewModel>> ModalStack { get; }

    IObservable<IImmutableList<IRoutableViewModel>> PageStack { get; }

    IView View { get; }

    IObservable<Unit> PopModal(bool animate = true);

    IObservable<Unit> PopPage(bool animate = true);

    IObservable<Unit> PopTo<TViewModel>() where TViewModel : IRoutableViewModel;

    IObservable<Unit> PopToRootPage(bool animate = true);

    IObservable<Unit> PushModal(IRoutableViewModel modal, string contract = null);

    IObservable<Unit> PushPage(IRoutableViewModel page, string contract = null, bool resetStack = false, bool animate = true);

    IObservable<IRoutableViewModel> TopModal();

    IObservable<IRoutableViewModel> TopPage();
}

Discussion

I know there are benefits to the current RoutingState implementation, I want to make this an option for stack based navigation platforms.

Questions

How do we handle platforms that don't have a concept of a modal stack?

If we implement passing parameters does it make sense for all platforms?

Do we allow consumers to implement their own IRoutingStack and register it for use with ReactiveUI?

Are there any performance concerns or improvements that could come from this change?

@anaisbetts
Copy link
Member

A few thoughts on this:

  1. It seems like ModalStack and PageStack act more like Behaviors (i.e. properties that always have a current value), so it would probably be way more ergonomic if IRoutingStack was a ReactiveObject that you can WhenAny on. This simplifies TopModal and TopPage too because it can just be an OAPH

  2. A lot of these fancy list types just get in the way, a regular IEnumerable is probably fine here (or IReadOnlyList maybe). tbh I'd probably just use a regular List.

  3. While Xamarin lets you have a stack of modal dialogs, is that really a Good Idea from a UX perspective? I would argue that you should have exactly One modal dialog slot active at any time, and that calling PushModal when a Modal is already active should just replace the previous one completely.

Do we allow consumers to implement their own IRoutingStack and register it for use with ReactiveUI?

I'd argue RoutingStack is more like List or String - it's really just a Bag of Data, so people shouldn't need to write a custom implementation (just like we don't have IString). People will ask for it, and you should Tell Them No :) Instead, make RoutingStack so easy to construct (i.e. with a constructor that optionally lets you fill in the stack explicitly for test time) and hackable that people won't need to.

@RLittlesII
Copy link
Member Author

@paulcbetts The moments where words matter. IRoutingStack should have been IRoutingStackService or IRouter.

I agree that the collection of data can be as basic as possible.

  1. ... I would argue that you should have exactly one modal dialog slot active at any time, and that calling PushModal when a Modal is already active should just replace the previous one completely.

I agree with there should only ever be a single modal present at a time. I am not sure that ReactiveUI should impose a limit in it's implementation that doesn't exist on the target platform.

@glennawatson
Copy link
Contributor

I would seperate the concepts of modal and navigation into two instances of the same interface. That way it's easier for the user to add new stacks to support y platform.

@glennawatson
Copy link
Contributor

Also reduces the number of methods in your interfaces eg navigation.Modal.Pop(true)

@glennawatson
Copy link
Contributor

  • As discussed offline it would be useful if we can separate out the concept of "animate" to it's own interface.
  • Support for Push and remove as per https://github.com/reactiveui/ReactiveUI/issues/1856
  • Why IObservable over ReactiveCommand's? ReactiveCommand's have re-entrance protection.

@BoutemineOualid
Copy link

BoutemineOualid commented Jun 12, 2019

Hello everyone,

I think these features are worth adding to the framework:

  • Navigate back with X hops.
  • Navigation back to a target vm already in the stack.
  • Navigation forward to a vm and removing x previous pages from the stack.

@glennawatson
Copy link
Contributor

Cool thanks @bouteminequalid I'm sure Rodney will take it into consideration

@RLittlesII
Copy link
Member Author

@BoutemineOualid All of these I believe are already captured on the Sextant Repository . If not feel free to make sure they are captured.

@mediabuff
Copy link

mediabuff commented Jan 13, 2021

  1. The API seems to lack 'forward view' stack. Platforms like WPF - frame based navigation support these
  2. Additionally, these platforms persist the 'view' state/visual tree - on their corresponding stacks. Thus the there are two stack model - one based on identity and other based on content. Which would the 'view model' navigation support ? is the view model re-created or cached ?
  3. Don't think there should be separate stack for modal and non-modal. The modal entries should stack on non-modal.
  4. what about memory cleanup - either of the views or view models in all scenarios

I have a working WPF version, based on WPF-Frame.

@RLittlesII
Copy link
Member Author

@mediabuff

  1. I am not sure I understand. I've not worked much with WPF
  2. My thought would be the content. The ViewModel should be persisted as the binding context? I wouldn't want to recreate that state everytime you Pop from the stack.
  3. This initial API was based on Xamarin's INavigation interface. I think WPF doesn't care, and @glennawatson has pointed out that WPF would prefer these concerns separated out because it doesn't make sense.
  4. Currently there is support for Destroying ViewModels in Sextant. I have been asked to open support for Views as well. I think providing an extension point for clean up is extremely important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants