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

Master detail control #452

Closed
hermitdave opened this issue Oct 5, 2016 · 78 comments
Closed

Master detail control #452

hermitdave opened this issue Oct 5, 2016 · 78 comments

Comments

@hermitdave
Copy link
Contributor

hermitdave commented Oct 5, 2016

higsecone_masterdetail
Toolkit and community would benefit tremendously if we had a control that implemented the master detail pattern described here
https://msdn.microsoft.com/en-gb/windows/uwp/controls-and-patterns/master-details

Discussing online with @ScottIsAFool @Depechie @bartlannoeye and Shawn Kendrot and moving it here.

@hermitdave
Copy link
Contributor Author

@Depechie all yours

@hermitdave hermitdave added feature 💡 help wanted Issues identified as good community contribution opportunities controls 🎛️ open discussion ☎️ labels Oct 5, 2016
@hermitdave hermitdave added this to the v1.2 milestone Oct 5, 2016
@Depechie
Copy link
Contributor

Depechie commented Oct 5, 2016

Well I'll share the details how we did it in https://github.com/AppCreativity/Kliva
It's not a control for now though.

We added our own ApplicationFrame, this to handle the SplitView control, because on Side-By-Side view we show the SplitView, in Stacked mode we show bottom appbar. We check this with view width.

Secondly our main page will act like a shell, so has 2 controls on it, the list of data and the details of a selected item in the list, shown side by side.
If we are on a small device, we hide the details control and trigger a page navigation to show the details.

That's it basically

@bartlannoeye
Copy link
Contributor

Note that we also use the Connected Animation on smaller devices when we navigate.

@Code-ScottLe
Copy link
Contributor

Code-ScottLe commented Oct 11, 2016

Here is another implementation if you need inspiration (does not use SplitView):
https://github.com/Code-ScottLe/USCISTracker

if we are on a small device, we hide the details control and trigger a page navigation to show the details.

on which frame that the detail would be displayed? The custom frame that the control uses or the main frame that the application use?

@skendrot
Copy link
Contributor

It would be nice to not have to navigate to another page to view the details if on smaller devices. Instead "fake" a navigation. I have a MasterDetailsPanel that I wrote a year ago. I'll find it and give it some polish.

@Code-ScottLe
Copy link
Contributor

It is true to not have to navigate to another page. Repeated code and data transitions were a pain.

@Depechie
Copy link
Contributor

Depechie commented Oct 14, 2016

Hmm by use of user controls and MVVM we have almost 0 code duplication and no faking navigation, you'll have to fake all animation transitions and we all know now those can change when switching WP OS version :P

@Code-ScottLe
Copy link
Contributor

Code-ScottLe commented Oct 14, 2016

When i first implemented this , i literally had to copy and pasted the code from the detail section in the Master-Detail page, onto the Detail page, just because I was too dumb to start the implementation using only Grid.

Using Splitview and collapse the panel on mobile for detail section makes much more sense imo.

@ScottIsAFool
Copy link
Contributor

I'm knocking something up for this at the moment

@ScottIsAFool
Copy link
Contributor

Well, I have a working control that behaves exactly as expected (albeit with no animations right now), but it doesn't work in the sample app because the sample app has a messed up navigation approach where a control can't prevent a back navigation because it can't access the frame being used.

@skendrot
Copy link
Contributor

I have a control with animations. I ha e tested navigation within my own app, but I have not tested it within the sample app. I can test that later today.

@ScottIsAFool
Copy link
Contributor

@skendrot I suspect whatever mechanism you have in place will also fall over with the sample app.

@skendrot
Copy link
Contributor

skendrot commented Oct 17, 2016

I think that a simply approach is needed for a control like this. The vast majority of use cases will only need a List for the "master". We would want to make it easy for beginner/intermediate developers to use these controls.

Compare the two examples:
Option A:

<controls:MasterDetailsView ItemsSource="{Binding Items}"
                            ItemTemplate="{StaticResource ListTemplate}"
                            DetailsTemplate="{StaticResource DetailsTemplate}"/>

Option B

<controls:MasterDetail x:Name="MasterDetail" DetailsTemplate="{StaticResource DetailsTemplate}">
    <controls:MasterDetail.Master>
        <ListView x:Name="ItemsList"
                  SelectionChanged="ItemsList_OnSelectionChanged"
                  ItemTemplate="{StaticResource ListTemplate}"/>
    </controls:MasterDetail.Master
</controls:MasterDetail>

And code behind for Option B:

// Could probably use binding within the definition in xaml
private void ItemsList_OnSelectionChanged(object sender, SelectionChangedEventArgs e)
{
    MasterDetail.Detail = e.AddedItems.FirstOrDefault();
}

Providing an easy to use control that allows for advanced developers to style and add additional features covers all cases.

EDITED (Scott): updated option B to show the current required code as visual states are no longer required to use the control.

@skendrot
Copy link
Contributor

I do appreciate the NoSelectionView property of @ScottIsAFool's, but would this be used often? I can't recall of an app that has something like this in it.

@deltakosh
Copy link
Contributor

Is OptionB actual user code?

@deltakosh
Copy link
Contributor

I do appreciate the easiness of option A which is clearly aligned with Toolkit philosophy.

@skendrot
Copy link
Contributor

It's a condensed version of the sample page provided for Option B

@deltakosh
Copy link
Contributor

This is definitely one point for OptionA ;D
Why not adding support for NoSelectionView? it seems fairly important to me

@Depechie
Copy link
Contributor

At first I was also very happy with the small footprint of option A!
But looking in more detail to my own app where I want to use it, we have a 'filter' list dropdown above the ListView in the Master part. So maybe extend with some HeaderTemplate option?

@deltakosh
Copy link
Contributor

The plan is to be able to use the control in 99% of the time so a footerTemplate will be required then

@skendrot
Copy link
Contributor

skendrot commented Oct 17, 2016

I like the idea of a ListHeader and ListHeaderTemplate. @deltakosh can you name any iOS/Android or windows apps that tell the user "No selection" or something similar? I have not seen any, but I also don't use a lot of apps

@deltakosh
Copy link
Contributor

Mine :) (UrzsaGatherer and Cast). When there is no selection I display a text to give user some feedback or guidance

@Depechie
Copy link
Contributor

screenshot 4
Filter and empty detail

@ScottIsAFool
Copy link
Contributor

@deltakosh the only thing I can do is create another control that uses my MasterDetail that is purely ListView based, although to be perfectly honest, I really don't see how my implementation is "difficult" and why it needs to be cut down any more than what it is right now. When you compare #452 (comment)

@deltakosh
Copy link
Contributor

Oh you removed all the VSM? This is completely different then!

Would love to get other voice here as I hate cutting a PR (And we need to cut one...)

@ScottIsAFool
Copy link
Contributor

@deltakosh I removed that ages ago, and told you I had ;) btw, the snap points are still customisable as they were before, you just don't need the visual states.

@deltakosh
Copy link
Contributor

I know..I'm underlining it for the community!

@deltakosh
Copy link
Contributor

I don't want to choose alone.This is not how the toolkit should work. It should be a decision coming from the majority of contributors/developers.

My vote goes to your PR because of flexibility and because it became simple enough to reach our min bar

@Depechie
Copy link
Contributor

Not tested code yet, but in our Kliva app we switch the splitview over to phone page bottombar on master and detail. Would this still be possible? I would think yes but with some plumbing?

@ScottIsAFool
Copy link
Contributor

@Depechie sorry, I don't follow, do you mean you hide the splitview pane, and show a bottom appbar?

@Depechie
Copy link
Contributor

Yeah and a different one on master and detail

@ScottIsAFool
Copy link
Contributor

You could bind to the Masterdetail.VisibleDisplay property to change appears.

@skendrot
Copy link
Contributor

I'm certain that if Microsoft were developing a MasterDetailsView control that it would inherit from Selector, or possibly ListViewBase. The Master pane would have a default implementation. Would that implementation be a ListView exactly? Probably not, but it would behave much like a ListView.

In software development you optimize for the 80% use case. In this situation, the 80% use case (probably actually much higher) is to display a list of items. For the rest there is styling. I'm not sure when styling a control became some difficult, advanced concept. It is one if the number one benefits of XAML. Restyling is the flexibility that is given to XAML developers to solve the 5-20% use cases.

Unfortunately, I do not believe that it is possible to add a default view to the master section and allow it to be customized without styling. Adding either a MasterTemplate or MasterControl property to the MasterDetailsView and having a default implementation makes it impossible to properly bind the ItemsSource, ItemTemplate, etc to the control itself without ancestor RelativeSource binding. Using TemplatedParent brings it to the ContentPresenter being used to display such a property.

As it seems the consensus is to require the 80-90% use case to add their own master view. I will close my PR.

@ScottIsAFool
Copy link
Contributor

Ok, so here's a question that @Depechie may be able to help answer: what is the story on iOS? As they have a MasterDetail control built into the SDK.

@deltakosh
Copy link
Contributor

Don't close yet. We need more voices to weight in here

@Depechie
Copy link
Contributor

Just a control with 2 placeholders for content https://developer.xamarin.com/guides/xamarin-forms/user-interface/navigation/master-detail-page/ but this tends to be more like the split view control! So the actual use case would be menu like system and not true master detail I guess

@ScottIsAFool
Copy link
Contributor

@Depechie is that the same for xamarin.ios too?

@Code-ScottLe
Copy link
Contributor

Code-ScottLe commented Oct 19, 2016

I think we need to get on the same page of what "Master-Detail" means and the mentality of the developers when they decide to do such a thing. I saw a lot of opinions on the case of allowing more freedom on what Master could be (not just ListView). By the end of the day, Master-Detail is not a control, it is a technique/pattern, even the official guide said this:

The master/details pattern works well if you want to:

Build an email app, address book, or any app that is based on a list-details layout.
https://msdn.microsoft.com/en-us/windows/uwp/controls-and-patterns/master-details

Also, this toolkit already has GridSplitter and the Framework already has SplitView, which both allow an extreme degree of freedom for displaying 2 contents at the same time that are related to each other.

@nmetulev
Copy link
Contributor

Agree with @Code-ScottLe and my vote is still for @skendrot's control. It doesn't cover all use cases but it makes it easier for the majority of cases.

@ScottIsAFool's control is great for giving more flexibility to the dev, but not enough as using a combination of the existing controls in the toolkit/framework would provide (It might as well be just a Detail control).

@deltakosh
Copy link
Contributor

@Code-ScottLe so your vote is for option A correct?

@Code-ScottLe
Copy link
Contributor

@deltakosh To be honest, the option A leaves a lot to be desired, but if i have to choose, it will have to be that one. It is clean, it is simple, straight to the point and align nicely with the official guideline - something that dev would expect after reading off what master-detail is.

From that point on, if they need something more advance, using SplitView to do their own thing would be reasonable imo.

@deltakosh
Copy link
Contributor

Sounds like we have a majority here for option A (for now)

@dotMorten
Copy link
Contributor

dotMorten commented Oct 19, 2016

Just a thought: What if the outer part of the master detail part was a Frame: The internals would just navigate to a page that shows the details with the details template, but that also allows the user to navigate within the frame to another page, or navigate to a custom view using myDetailsView.NavigateTo(someOtherPage) ? We also get the benefit of a backstack inside the master details view.

@skendrot
Copy link
Contributor

I had thought about making the details panel a frame but decided against for two reasons

  • The app probably already has a frame so now it would have two that the user might have to manage
  • It would mess up frameworks that have a NavigationService.

@hermitdave
Copy link
Contributor Author

hermitdave commented Oct 20, 2016

Frame navigation is messy.. From what I have seen frequent navigation leads to memory and handle allocations that don't get freed up fast enough.

From the choice, If I was consuming the app, I would prefer the simpler version. Specify ItemSource, Master Item and Detail templates and bingo.

No messing about in the code. No handling selection events etc.

However. We should be able to also set Items directly not just through ItemsSource

@deltakosh
Copy link
Contributor

Ok so we are heading to:

  • Option A
  • Adding the Map function for advanced need

Sounds reasonable to all? (democracy FTW)

@skendrot
Copy link
Contributor

OK. so the last item would be to allow for the following:

public MainPage()
{
    InitializeComponenet();
    MyMasterView.MapDetailAsync = GetComplexModel;
}

private Task<object> GetComplexModelAsync(object obj)
{
    var model = obj as Model;
    if(model == null) return;

    return Task.FromResult(new ComplexModel(model));
}

And that's better than the following:

// assumes binding of SelectedItem from XAML
public Model SelectedItem 
{
    get { return _model; }
    set
    {
        _model = value;
        if(_model != null)
        {
            _model.ExtraData = GetExtraData();
        }
    }
}

Would this be something to add as it's own issue? Do more people want to weigh in? I've seen four people talk about it and it looks split 2-2 on implementing it. I'm ok making the change and I have something that works. Just want to make sure that's the consensus.

@deltakosh
Copy link
Contributor

Like it. Go ahead please using this PR

@skendrot
Copy link
Contributor

Thoughts on the mapping function being async? I'm leaning toward synchronous and letting to user do async work themselves. This allows the details panel to have content that the user can show (loading text) while they are getting the extra info. Making it Async would cause the detail area to be blank unless the user restyled it to show. One possibility for async is to add a loading template that is displayed when getting the mapped data.

@deltakosh
Copy link
Contributor

Keep things simple:) synchronous is fine

@ghost ghost locked as resolved and limited conversation to collaborators Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants