-
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
ListDetailsView now uses the Two-Pane View #4197
ListDetailsView now uses the Two-Pane View #4197
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.
We should update the names of a few of the properties and parts.
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.Properties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.xaml
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.BackButton.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.BackButton.cs
Outdated
Show resolved
Hide resolved
} | ||
else if (previousState == ListDetailsViewState.Details) | ||
{ | ||
if ((BackButtonBehavior == BackButtonBehavior.Inline) && (_inlineBackButton != null)) |
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.
Same as above, extra parentheses
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.BackButton.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.BackButton.cs
Outdated
Show resolved
Hide resolved
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Looking good to me! Think only thing missing (which may have been from the WinUI 2.6 transition) is that there's no visual edge between the two sides, like in the previous we had a line: We may just want a different coloring or something on one side? Not sure what the best practice would be here. We can fix this outside this PR though and open an issue. |
Thanks @COM8 for these improvements! Sorry it took so long for us to move forward on them, but appreciate you working with us on getting everything together to be integrated. 🎉🎉🎉 |
@JustinXinLiu any comments on styling choices? I think aligning more with the 2.6 WinUI updates to NavigationView would make sense, eh? |
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The ListDetailsView does not take advantage of multiple screens and is not spanning aware.
What is the new behavior?
The new and refactored ListDetailsView control is aware of multiple screens.
We archive this by replacing the current
grid
-based layout with the new Two-pane view based layout.Additional changes
DetailsPaneBackground
,DetailsContentTemplateSelector
,ListPaneEmptyContent
,ListPaneEmptyContentTemplate
andListPaneItemTemplateSelector
.PR Checklist
Please check if your PR fulfills the following requirements:
Other information
Old behavior
New behavior