-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MasterDetailsView Windows 10X and Two-pane view support #3173
Conversation
Thanks COM8 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
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.
This PR is very difficult to review within GitHub due to items being moved around. Please move properties, methods, member variables, etc. to their original locations
@@ -15,6 +15,26 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls | |||
/// <seealso cref="Windows.UI.Xaml.Controls.ItemsControl" /> | |||
public partial class MasterDetailsView | |||
{ | |||
/// <summary> |
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.
It looks like properties have been moved around. This is making it really difficult to see what actually changed. Can you put the properties back in the order they were originally.
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.
OK. I will work on it tomorrow.
I've rearranged them to make them a little more logical.
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 could be done as a separate "no functional change" PR
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.
Ok. I've rearranged them.
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
// Control names: | ||
private const string PartRootPane = "RootPane"; | ||
private const string PartDetailsPresenter = "DetailsPresenter"; | ||
private const string PartDetailsPane = "DetailsPane"; |
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.
Do not change the names of the panels in the view
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.
Fixed
I have unchecked the |
@michael-hawker OK, good to know. |
Just adding do not merge label so we can keep track with our WinUI 3 work later. |
Converting to Draft as we need to wait until we're working towards WinUI 3 and take the WinUI dependency. |
@COM8 just wanted to give you an update since it's been a while and the WinUI 3 shift has changed some of our plans in the Toolkit. I think in one of our upcoming releases this year, we'll be considering adding a dependency on WinUI 2.x. At that time we can bring this PR forward and get it merged in. I'll keep you posted. |
Cool. Thanks for the update. |
@COM8 we have decided to take a dependency on WinUI 2.x. So this PR doesn't have to wait for any WinUI 3 work anymore. It's too late for our upcoming 7.0, but we can get it scheduled for a future release. We have done some refactors since moving controls around, so this will need to be rebased/updated on the current main branch. Was there many things left or was this otherwise ready to go, once things are rebased? I think the main thing for us will just be how we time releasing it if we're significantly changing the API surface of the control itself. If it's mainly style/internal changes, we'll have to evaluate if we want that in a minor release or not. |
Glad to hear! |
Otherwise the PR and documentation MicrosoftDocs/WindowsCommunityToolkitDocs#304 were ready to go. |
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The MasterDetailsView does not take advantage of multiple screens on Windows 10X devices.
What is the new behavior?
The new and refactored MasterDetailsView control is aware of multiple screens when used on Windows 10X devices.
We archive this by replacing the current
grid
-based layout with the new Two-pane view based layout.Additional changes
DetailsPaneBackground
,DetailsContentTemplateSelector
,MasterNoItemsContent
,MasterNoItemsContentTemplate
andMasterItemTemplateSelector
.Microsoft.Toolkit.Uwp.UI.Controls
so I would be able to use the TwoPaneView class regardlessof the current Windows 10 version. Need feedback here!TargetPlatformVersion
from10.0.17763.0
to10.0.18362.0
bug_report.md
template to reflect these changes.PR Checklist
Please check if your PR fulfills the following requirements:
Other information
Old behavior
New behavior