-
Notifications
You must be signed in to change notification settings - Fork 711
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
NavigationView InfoBadge integration #5769
Conversation
…o user/stpete/NVIInfoBadgeSupport
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…22 icon column width instead of 8 to account for the changes made in the PR, update Content margin to account for changed icon column width.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -135,7 +140,7 @@ void NavigationViewItemPresenter::UpdateCompactPaneLength(double compactPaneLeng | |||
const auto gridLength = compactPaneLength; | |||
|
|||
templateSettings->IconWidth(gridLength); | |||
templateSettings->SmallerIconWidth(gridLength - 8); | |||
templateSettings->SmallerIconWidth(gridLength - 22); |
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 this change for?
@@ -294,7 +294,7 @@ | |||
<Thickness x:Key="NavigationViewCompactItemSeparatorMargin">0,3,0,4</Thickness> | |||
<Thickness x:Key="TopNavigationViewItemSeparatorMargin">3,0,4,0</Thickness> | |||
<Thickness x:Key="TopNavigationViewOverflowButtonMargin">0</Thickness> | |||
<Thickness x:Key="NavigationViewItemContentPresenterMargin">4,-1,20,-1</Thickness> | |||
<Thickness x:Key="NavigationViewItemContentPresenterMargin">18,-1,20,-1</Thickness> |
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 question as the template settings smallerIconWidth comment.
</Grid.ColumnDefinitions> | ||
|
||
<Border x:Name="IconColumn" Width="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=TemplateSettings.SmallerIconWidth}"> | ||
<Viewbox x:Name="IconBox" | ||
Height="{ThemeResource NavigationViewItemOnLeftIconBoxHeight}" | ||
HorizontalAlignment="Center" | ||
HorizontalAlignment="Right" |
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.
Would this change break if we set the iconColumnWidth wider?
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.
Yes your right... This and the other two changes you called out are to get around a rendering issue(feature?) where if the icon column is larger than the available size the parent grid would impose a clip. This happens because of a margin I had to introduce to get the info badge to not overlap the scroll bar..... This isn't usually an issue because there are 14 pixels of extra space to the right of the icons which we can remove for the scrollbar and just align the icon to the right and reduce its size.
But since this doens't work when you change the sizes... I'll find a different way to do this
@@ -797,6 +815,13 @@ | |||
</local:AnimatedIcon.RenderTransform> | |||
</local:AnimatedIcon> | |||
</Grid> | |||
|
|||
<ContentPresenter |
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 should be moved up, since its grid.Column is 2
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 want the badge to render above all the other UI components so it should be declared last, usually it wont matter because the grid columns don't overlap but you can make them with some of the exposed resources.
@@ -1393,6 +1449,9 @@ | |||
</local:AnimatedIcon.RenderTransform> | |||
</local:AnimatedIcon> | |||
</Grid> | |||
|
|||
<ContentPresenter x:Name="InfoBadgePresenter" Grid.Column="2" VerticalAlignment="Center" Content="{TemplateBinding InfoBadge}"/> |
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 should also be moved before expand Collapse chevron.
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 want the badge to render above all the other UI components so it should be declared last.
…the margin during the expanded state
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -33,6 +33,8 @@ class NavigationViewItemPresenter: | |||
|
|||
void UpdateClosedCompactVisualState(bool isTopLevelItem, bool isClosedCompact); | |||
|
|||
void UnparentInfoBadge(); |
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.
can remove, no implementation
@@ -294,17 +294,17 @@ | |||
<Thickness x:Key="NavigationViewCompactItemSeparatorMargin">0,3,0,4</Thickness> | |||
<Thickness x:Key="TopNavigationViewItemSeparatorMargin">3,0,4,0</Thickness> | |||
<Thickness x:Key="TopNavigationViewOverflowButtonMargin">0</Thickness> | |||
<Thickness x:Key="NavigationViewItemContentPresenterMargin">4,-1,20,-1</Thickness> | |||
<Thickness x:Key="NavigationViewItemContentPresenterMargin">4,-1,8,-1</Thickness> |
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.
Have you verified that everything looks correct visually even in other states?
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.
Discussed offline. Surfaced existing visual bugs that will be filed separately.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Add Info Badge integration into Navigation View: