Add padding to the NavigationViewItem to avoid occluding expand/collapse chevron #2342
Conversation
msft-github-bot
added
the
needs-triage
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
@Felix-Dev could you also upload a screenshot of how this change looks when in a flyout?
|
@chigy for FYI, screen shots of what the adjusted margins look like. |
|
Perhaps the added 6px too much to the left 🤔 |
@Felix-Dev , I am not sure what I am looking at. Can you share a screenshot of wider view? |
|
@chigy The right padding we decided on in the discussion thread for the chevron is also applied here (even though there is no scrollbar in the flyout). |
@Felix-Dev , that's what I thought but wanted to make sure. Thanks.
Why was it applied here? Is it using the same value? I did not expect padding to impact this. |
|
@chigy Yes, the theme resources we updated (e.g. I'm currently looking at the code how to best apply the original padding when we are showing the items in a flyout. I would like to do that in a VisualState. My current thinking is - and this might be wrong - to apply our updated paddings only whe the NavigationView is not in the LeftCompact state. When the NavigationView is in LeftCompact only then are we showing the child items in a flyout. When the pane is expanded we show the child items integrated in the NavigationView pane. (This is only talking about Left display mode which is affected by the padding changes, not Top display mode.) Ideally I would thus have some VisualStates for the |
ranjeshj
added
area-NavigationView
|
@Felix-Dev , I found out that the flyout version does indeed display scrollbar when it is long enough so this is unfortunate but looks like by design today until such time we can revisit the scroll bar design. |
|
@chigy I see. Hmm, that's a bit unfortunate then. But I guess it's still better to have the chevron not being party occluded than to not have that unnecessary spacing in the flyout when there's no scrollbar. |
…th-contentpresenter-alignment
|
Updated the visual tree masters. Although on my local system, the test in question (NavigationViewTests.VerifyVisualTree()) still failed (apparently when trying to verify brush colors?). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@chigy are you signing off on this change? |
|
Moving conversation about cutoff text here. @chigy this is what it looks like when excluding the changes being made in this PR.
|
|
@ojhad , thanks. Unfortunate... It does look more cutoff but doesn't make difference in my signing the change off so will do so. |
|
@ojhad Yes, I'm working on this currently for this PR. I have also noticed that we have the same issue in PaneDisplayMode::Top when we display menu items with a chevron in the flyout:
|
Awesome!
Yep, that would make sense. The presenter style for those items is defined under |
|
@ojhad and @Felix-Dev its been about a week since this got any traction. What are the next steps we need? |
|
@StephenLPeters I was away over the weekend (we had a long weekend over here) but I'm currently working on finishing this PR. I apologize for any inconvenience this break might have caused. |
No appologies needed @Felix-Dev I simply wanted to check in on the status :). Thank you so much for helping out with this issue. |
* Restored original ContentPresenter margin for top-level items when the pane is in compact mode * Paddings have been updated to prevent the expand/collapse chevron from being partly occluded by the expanded scrollbar when in Top PaneDisplayMode for items displayed in flyouts (overflow menu and child items)
|
As suggested, I added a
Padding now has also been added to NavigationViewItems in Top PaneDisplayMode when displayed in flyouts (children flyout and overflow flyout). The expand/collapse chevron now has a 4px spacing between itself and the expanded scrollbar - the same as for the different Left* PaneDisplayModes:
This commit also adds another theme resource: The values for Top PaneDisplayMode theme resources have been modified in such a way to
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@ojhad @chigy Regarding the cutoff text, with the work done here, we can now also restore the cutoff text look to like it was prior to WinUI 2.4 (with the hierarchical NavView update). WinUI 2.3 cutoff look: If we now set our newly introduced theme resource like this: <Thickness x:Key="NavigationViewCompactItemContentPresenterMargin">0</Thickness>we get that previous look too: As we now, however, no longer use any right padding, the expanded scrollbar will now occlude the content as much as possible: The request to restore the pre WinUI 2.4 cutoff text in closed compact mode is tracked in issue #2541 and we could work on that as soon as this PR has been merged and you think the original padding (zero padding to the right) should be restored. |
|
@Felix-Dev , as far as I am aware, it was never intention for us to change the previous behavior with the update we made, so it does make sense to make it so that it behaves the same way it did. |
|
@chigy As soon as this PR is merged, I'll create a PR to restore the previous WinUI 2.3 cutoff text look then. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Seems like we need to start another run: |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Felix-Dev
deleted the
user/Felix-Dev/navviewitem-padding-6px-with-contentpresenter-alignment
branch
Description
As discussed here this PR adds some right padding to the expand/collapse chevron (its hit area) to ensure there is a 4px space between the chevron and the expanded scrollbar.
The PR also adds an additional right padding to the NavigationViewItem's ContentPresenter to ensure it is visually aligned with the chevron.
Motivation and Context
Closes #2287
How Has This Been Tested?
Tested visually
Screenshots