-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New MasterDetailsView control to follow the Master/Details pattern #483
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
Conversation
Hi @skendrot, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
@skendrot, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
My initial thoughts on this are that it doesn't allow for non-lists in the master section. For example, a settings page. |
The vast majority of use cases involve a ListView. Even the Windows Settings app. The Settings app just happens to also have a search bar and title bar in the "master" section. Better to have someone restyle to put a search bar than have everyone put a list in. |
6b05f65
to
1ca4bf4
Compare
…closing the master pane when in a narrow state. Use Composition animations to animate the details in out.
…does so the app does not navigate back when the event is already handled
…s a header for the master area
…s in narrow state
… control itself to get an accurate size of the presenter
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; |
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.
Remove redundant usings
new Email { From = "OneDrive", Subject = "Check out your event recap", Body = "Your new album.\r\nYou uploaded some photos to yuor OneDrive and automatically created an album for you."}, | ||
new Email { From = "Twitter", Subject = "Follow randomPerson, APersonYouMightKnow", Body = "Here are some people we think you might like to follow:\r\n.@randomPerson\r\nAPersonYouMightKnow"}, | ||
}; | ||
this.InitializeComponent(); |
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.
Remove instances of "this."
<TextBlock Text="{Binding From}" Style="{ThemeResource SubtitleTextBlockStyle}" RelativePanel.RightOf="FromEllipse" Margin="12,-6,0,0"/> | ||
<TextBlock x:Name="SubjectLine" Text="{Binding Subject}" Style="{ThemeResource BodyTextBlockStyle}" RelativePanel.Below="FromEllipse"/> | ||
<TextBlock x:Name="Body" Text="{Binding Body}" Style="{ThemeResource BodyTextBlockStyle}" TextWrapping="Wrap" RelativePanel.Below="SubjectLine" Margin="0,12,0,0"/> | ||
<Button Content="Click ME!" RelativePanel.Below="Body"/> |
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.
What's the purpose of this button?
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.
the purpose is to click it ;)
I think I was going to add reply/forward/etc buttons but decided against it. Will be removed
{ | ||
var view = (MasterDetailsView)d; | ||
string noSelectionState = view._stateGroup.CurrentState == view._narrowState | ||
? "NoSelectionNarrow" |
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'd be happier if VisualState names/groups and Template child names were constants.
_compositor = _root.Compositor; | ||
|
||
_detailsPresenter = (ContentPresenter)GetTemplateChild("DetailsPresenter"); | ||
_detailsPresenter.SizeChanged += OnSizeChanged; |
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.
If I change the template and change the details presenter in some way (change its name, remove it, whatever), this will crash the control. Should have a null check.
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.
That's intended. The View doesn't function the same without it. Would need to check many other places for null as well. Others have thoughts?
</VisualState> | ||
<VisualState x:Name="WideState"> | ||
<VisualState.StateTriggers> | ||
<AdaptiveTrigger MinWindowWidth="720" /> |
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've never been able to track down what the true "best practice" is on window width breakpoints, and it seems everyone does it however they want. That being said, the Windows guidelines pretty clearly denote breakpoints of 640 and below as narrow, 641 to 1007 as medium, and 1008 and greater as large. I'm not sure where these values came from, but they do suggest apps follow them. So, following that, it seems this value should be 641.
To make things more confusing though, the diagram directly above it show different breakpoints, and the header at the top shows a different set of breakpoints altogether.
Thoughts from the Microsoft folks? (@deltakosh, etc)
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.
What's the status on this?
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 would say follow the guidelines @jneidlinger pointed to.
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.
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.
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.
@jneidlinger it's the row below it he needs, not the one you circled :)
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.
@ScottIsAFool Fixed. Halloween. :)
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.
Well - I did some more digging, and the specific guidelines for master / detail view do call out using 720 as the breakpoint. I wish the guidelines were a little more consistent.
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.
Haha, looks like it's good to stay as is then.
Conflicts: Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj
Still need a doc file. Need to record a gif of the control |
@skendrot didn't you already record one https://twitter.com/skendrot/status/787899417487474689? |
No. I uploaded a video to twitter, they turned it into a gif. And I'd rather not use that gif for the repo |
Fix the conflict and we are good to go |
Conflicts: Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj
Are we still waiting for documentation for this? |
Nope we have it |
Ah, sorry, thought the tag was still there :) |
YEAHHH!Congrats @skendrot ! |
Addresses issue #452. Allows for the user to follow the Master/Details design pattern. Solves the problem of the user having to setup navigation to a details page if on a small device.
Using the control is simple. User sets the ItemsSource, ItemTemplate, and DetailsTemplate