Hierarchical Navigation View #2004
Conversation
msft-github-bot
added
the
needs-triage
ojhad
changed the title
| { | ||
| if (auto grid = element.try_as<winrt::Grid>()) | ||
| { | ||
| return grid.Name() == c_flyoutRootGrid; |
There was a problem hiding this comment.
object comparison by a name property seems dangerous. who's to say there isn't a grid with this name?
There was a problem hiding this comment.
This is a good point. Currently this is the method that is being used to determine if an Item is being displayed in a flyout. I'll open a separate issue to update this so that it does not depend on the visual tree in order to gain this information.
There was a problem hiding this comment.
@ojhad Can you create an issue and link it here ?
- added selection indicator and chevron animations - fixed bugs in icon content, overflow selection, closing flyout
ojhad
force-pushed
the
user/ojhad/navigationviewhierarchical
branch
from
182e8f8 to
6615eed
Compare
ojhad
changed the title
| contract7Present:Placement="RightEdgeAlignedTop"> | ||
| <Flyout.FlyoutPresenterStyle> | ||
| <Style TargetType="FlyoutPresenter"> | ||
| <Setter Property="Padding" Value="0,8" /> |
There was a problem hiding this comment.
0,8 [](start = 74, length = 3)
ideally these magic numbers could be exposed as static resources
There was a problem hiding this comment.
Agreed, resources would be great. I expect a small but non-zero percentage of customers wanting to customize the flyout padding. Is there an existing resource we can reuse that logically makes sense?
There was a problem hiding this comment.
That's definitely is a good idea. As of now, I can't think of an existing resource that we could re-use here, as I styled this flyout to be like the overflow flyout in TopNav, which does not expose these properties.
There was a problem hiding this comment.
@ojhad @YuliKl @StephenLPeters We could add two padding theme resources - one resource for the children flyouts (used in both Left and Top mode) and the second resource used for the overflow menu in Top mode.
Why two resources instead of one? Logically, the content of both flyouts differ. One flyout lists the children for a menu item, whereas the overflow flyout lists distinct top-level items (including their children if any). As such it could make sense to give customers the ability to choose a different padding for these two different flyout types.
Proposed theme resources:
- TopNavigationViewOverflowMenuPadding
- NavigationViewItemChildrenMenuFlyoutPadding
I'm proposing using "MenuFlyout" instead of just "Menu" for the children flyout padding theme resource as children can also be displayed inline (in Left mode) and I'm not sure if NavigationViewItemChildrenMenuPadding would make it clear that this resource only affects the padding when the children are displayed inside a flyout and not inline. As such, I picked "MenuFlyout" to make it crystal clear when the padding will be applied.
I don't believe adding "Flyout" to the TopNavigationViewOverflowMenuPadding name is required as the documentation also only speaks of the "overflow menu" and not the "overflow menu flyout".
Thoughts?
| static constexpr auto c_enabled = L"Enabled"sv; | ||
| static constexpr auto c_normal = L"Normal"sv; | ||
| static constexpr auto c_chevronHidden = L"ChevronHidden"sv; | ||
| static constexpr auto c_chevronVisible = L"ChevronVisible"sv; |
There was a problem hiding this comment.
I think ideally you would include visual state names with these definitions
There was a problem hiding this comment.
Sorry, I don't get what you mean? These are visual state names.
There was a problem hiding this comment.
For instance you don't have these state names up here L"IconOnLeft" L"IconOnly" L"ContentOnly" names up here
In reply to: 394637672 [](ancestors = 394637672)
| if (auto rootGrid = GetTemplateChildT<winrt::Grid>(c_rootGrid, controlProtected)) | ||
| { | ||
| m_rootGrid.set(rootGrid); | ||
|
|
There was a problem hiding this comment.
this means m_rootGird wont get set to null if GetTemplateChildT fails. I think the lambda version is slightly better for this reason, look at numberbox.cpp for example. This would be okay to not fix though.
There was a problem hiding this comment.
m_rootGrid will be null when the object is created no ?
There was a problem hiding this comment.
Yep, that is the case. The lambda approach does seem better, however this is currently the approach we've been taking throughout this whole control. I think possibly creating a task to do a mass update might be a good idea.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ojhad
added
area-NavigationView
Description
Implemented functionality for multi level support.
These changes leverage the SelectionModel in order to support multi level selection.
Also this change includes breaking the NavigationViewItemBase dependency on ListViewItem and moving the pointer visual states logic into NavigationViewItem.
Motivation and Context
Part of task #1317
Part of task #79
How Has This Been Tested?
Manual testing and added some tests