Skip to content
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

Hide NavigationViewItem Chevron if Children is empty #2759

Merged
9 changes: 8 additions & 1 deletion dev/NavigationView/NavigationViewItem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,16 @@ void NavigationViewItem::UpdateRepeaterItemsSource()
}
return MenuItems().as<winrt::IInspectable>();
}();
m_itemsSourceViewChanged.revoke();
Copy link
Collaborator

@marcelwgn marcelwgn Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#Fixed I think you need to revoke this listener in UnhookEventsAndClearFields too:

void NavigationViewItem::UnhookEventsAndClearFields()
{
UnhookInputEvents();
m_flyoutClosingRevoker.revoke();
m_splitViewIsPaneOpenChangedRevoker.revoke();
m_splitViewDisplayModeChangedRevoker.revoke();
m_splitViewCompactPaneLengthChangedRevoker.revoke();
m_repeaterElementPreparedRevoker.revoke();
m_repeaterElementClearingRevoker.revoke();
m_isEnabledChangedRevoker.revoke();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I also need to rename the variable into m_itemsSourceViewCollectionChangedRevoker to be inline with the other revokers?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, yes that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a commit doing this.

repeater.ItemsSource(itemsSource);
m_itemsSourceViewChanged = repeater.ItemsSourceView().CollectionChanged(winrt::auto_revoke, { this, &NavigationViewItem::OnItemsSourceViewChanged });
}
}

void NavigationViewItem::OnItemsSourceViewChanged(const winrt::IInspectable& /*sender*/, const winrt::NotifyCollectionChangedEventArgs& /*args*/)
{
UpdateVisualStateForChevron();
}

winrt::UIElement NavigationViewItem::GetSelectionIndicator() const
{
Expand Down Expand Up @@ -468,7 +475,7 @@ void NavigationViewItem::UpdateVisualStateForChevron()

bool NavigationViewItem::HasChildren()
{
return MenuItems().Size() > 0 || MenuItemsSource() != nullptr || HasUnrealizedChildren();
return MenuItems().Size() > 0 || (MenuItemsSource() != nullptr && m_repeater.get().ItemsSourceView().Count() > 0) || HasUnrealizedChildren();
}

bool NavigationViewItem::ShouldShowIcon()
Expand Down
2 changes: 2 additions & 0 deletions dev/NavigationView/NavigationViewItem.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class NavigationViewItem :
bool HasChildren();

void UpdateRepeaterItemsSource();
void OnItemsSourceViewChanged(const winrt::IInspectable& sender, const winrt::NotifyCollectionChangedEventArgs& args);
void ReparentRepeater();
void OnFlyoutClosing(const winrt::IInspectable& sender, const winrt::FlyoutBaseClosingEventArgs& args);
void UpdateItemIndentation();
Expand All @@ -124,6 +125,7 @@ class NavigationViewItem :

winrt::ItemsRepeater::ElementPrepared_revoker m_repeaterElementPreparedRevoker{};
winrt::ItemsRepeater::ElementClearing_revoker m_repeaterElementClearingRevoker{};
winrt::ItemsSourceView::CollectionChanged_revoker m_itemsSourceViewChanged{};

winrt::FlyoutBase::Closing_revoker m_flyoutClosingRevoker{};
winrt::Control::IsEnabledChanged_revoker m_isEnabledChangedRevoker{};
Expand Down
65 changes: 65 additions & 0 deletions dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -759,5 +759,70 @@ public void VerifyNavigationViewItemInFooterDoesNotCrash()
Verify.IsTrue(true);
});
}

[TestMethod]
public void VerifyExpandCollapseChevronVisibility()
Copy link
Contributor

@Felix-Dev Felix-Dev Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • For now we check if the chevron is collapsed when the bound data collection is empty. We should probably also check in this test if the chevron visibility is updated accordingly when the NavigationViewItem.MenuItemsSource is set to null. (I.e. you could add an item again to the bound collection after our final visibility check after clearing the collection, then set parentItem.MenuItemsSource = null; and verify again if the chevron is collapsed.)

  • Right now we only use the NavigationViewItem.MenuItemsSource API to verify the correctness of the chevron visibility. For completeness, we could also add testing if the chevron visibility is updated correctly when using the NavigationViewItem.MenuItems API instead. Testing with this API would simply add/remove items to/from MenuItems (as MenuItems cannot be set to null) and check the chevron visibility accordingly. These checks could be added inside this method below our tests for the MenuItemsSource API (for example after clearing the bound collection or setting MenuItemsSource to null) so that we start testing using the MenuItems API with the chevron in collapsed state.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there no other tests that would verify the other branches of the HasChildren function? I thought that some other tests verified this implicitly.

Copy link
Contributor

@Felix-Dev Felix-Dev Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chingucoding I haven't immediately found any other test (API/Interaction test) which we could fall back to here.

Edit: Just noticed there is no public NavigationViewItem.HasChildren API so ignore what I previously wrote here about having to create a test for it too 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that are no tests that test collection changes in the MenuItems API. Would be best if we add those here or if we create a tracking issue to do that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am adding the tests for the MenuItems API now. Will push it once it passes the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests have passed even the tests for the MenuItems API.

image

{
NavigationView navView = null;
NavigationViewItem parentItem = null;
ObservableCollection<string> children = null;

RunOnUIThread.Execute(() =>
{
navView = new NavigationView();
Content = navView;

children = new ObservableCollection<string>();
parentItem = new NavigationViewItem() { Content = "ParentItem", MenuItemsSource = children };

navView.MenuItems.Add(parentItem);

navView.Width = 1008; // forces the control into Expanded mode so that the menu renders
Content.UpdateLayout();

UIElement chevronUIElement = (UIElement)FindVisualChildByName(parentItem, "ExpandCollapseChevron");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use VisualTreeUtils.FindVisualChildByName() here instead of having to provide an own implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also removed the FindVisualChildByName method.

Verify.IsTrue(chevronUIElement.Visibility == Visibility.Collapsed, "chevron should have been collapsed as NavViewItem has no children");

// Add a child to our NavigationView parentItem. This should make the chevron visible.
children.Add("Undo");
Content.UpdateLayout();

Verify.IsTrue(chevronUIElement.Visibility == Visibility.Visible, "chevron should have been visible as NavViewItem now has children");

// Remove all children from our NavigationView parentItem. This should collapse the chevron
children.Clear();
Content.UpdateLayout();

Verify.IsTrue(chevronUIElement.Visibility == Visibility.Collapsed, "chevron should have been collapsed as NavViewItem no longer has children");
});
}

private static DependencyObject FindVisualChildByName(FrameworkElement parent, string name)
{
if (parent.Name == name)
{
return parent;
}

int childrenCount = VisualTreeHelper.GetChildrenCount(parent);

for (int i = 0; i < childrenCount; i++)
{
FrameworkElement childAsFE = VisualTreeHelper.GetChild(parent, i) as FrameworkElement;

if (childAsFE != null)
{
DependencyObject result = FindVisualChildByName(childAsFE, name);

if (result != null)
{
return result;
}
}
}

return null;
}

}
}