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.

Sign up for GitHub

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
merged 7 commits into from
Jul 1, 2020
Merged

Hide NavigationViewItem Chevron if Children is empty #2759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1663ac7
Hide NavigationViewItem Chevron if Children is empty
MarkIvanDev Jun 26, 2020
7906e46
removed unused args
MarkIvanDev Jun 27, 2020
50577c7
add VerifyExpandCollapseChevronVisibility API test
MarkIvanDev Jun 30, 2020
98ff096
revoke m_itemsSourceViewCollectionChangedRevoker inside UnhookEventsA…
MarkIvanDev Jun 30, 2020
a56fa72
use VisualTreeUtils.FindVisualChildByName and removed own implementation
MarkIvanDev Jun 30, 2020
4750dce
add additional test for the MenuItems API
MarkIvanDev Jun 30, 2020
69fc1bf
add visibility check before setting MenuItemsSource as null
MarkIvanDev Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion dev/NavigationView/NavigationViewItem.cpp
View file
Open in desktop
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_itemsSourceViewCollectionChangedRevoker.revoke();
repeater.ItemsSource(itemsSource);
m_itemsSourceViewCollectionChangedRevoker = 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 Expand Up @@ -919,6 +926,7 @@ void NavigationViewItem::UnhookEventsAndClearFields()
m_repeaterElementPreparedRevoker.revoke();
m_repeaterElementClearingRevoker.revoke();
m_isEnabledChangedRevoker.revoke();
m_itemsSourceViewCollectionChangedRevoker.revoke();

m_rootGrid.set(nullptr);
m_navigationViewItemPresenter.set(nullptr);
Expand Down
2 changes: 2 additions & 0 deletions dev/NavigationView/NavigationViewItem.h
View file
Open in desktop
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_itemsSourceViewCollectionChangedRevoker{};

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

[TestMethod]
public void VerifyExpandCollapseChevronVisibility()
Copy link
Contributor

@Felix-Dev Felix-Dev Jun 30, 2020 •
edited
Loading

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 •
edited
Loading

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)VisualTreeUtils.FindVisualChildByName(parentItem, "ExpandCollapseChevron");
Verify.IsTrue(chevronUIElement.Visibility == Visibility.Collapsed, "chevron should have been collapsed as NavViewItem has no children");

// Add a child to parentItem through the MenuItemsSource API. This should make the chevron visible.
children.Add("Child 1");
Content.UpdateLayout();

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

// Remove all children of 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");

// Add a child to parentItem and set the MenuItemsSource as null. This should collapse the chevron
children.Add("Child 2");
Content.UpdateLayout();

// we are doing this so that when we set MenuItemsSource as null, we can check if the chevron's visibility really changes
Verify.IsTrue(chevronUIElement.Visibility == Visibility.Visible, "chevron should have been visible as NavViewItem now has children");

parentItem.MenuItemsSource = null;
Copy link
Contributor

@Felix-Dev Felix-Dev Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need a Content.UpdateLayout() call here to make sure the chevron is visible again before setting the MenuItemsSource to null and verifying that it actually collapses the chevron? (Before adding "Child 2" item, the chevron is collapsed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we have already checked earlier at Line 787 that adding an item to children does make the chevron visible right? I think that it would be redundant to make that check again here?

Copy link
Contributor

@Felix-Dev Felix-Dev Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking here is not to verify that adding an item actually makes the chevron visible (by adding an additional Verify.IsTrue(chevronUIElement.Visibility == Visibility.Visible)) but making sure that by the time we set parentItem.MenuItemsSource = null; the chevron is actually visible. That way, we can make sure that setting MenuItemsSource to null will indeed collapse the chevron.

In other words, my concern here is the following: Before we add our child item (Child 2) the chevron is collapsed. We then add this child and immediately after that set MenuItemsSource to null. I'm concerned that without the Content.UpdateLayout() call after adding the child, the chevron might not actually be made visible again, thus we are not actually verifying whether setting MenuItemsSource to null collapses the chevron. (Since the chevron might still be collapsed when we set the source to null and thus the Verify.IsTrue() call below succeeds, even though MenuItemsSource = null might not actually change the chevron visibility state at all.)

@StephenLPeters @chingucoding Your thoughts here?

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 understand now. Thanks. Will add those now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Valid concern, and another Verify would guarantee that was the case. Without it it could be the case that a bug causes the chevron to never show which wouldn't be caught without the "redundant" verify.

Content.UpdateLayout();

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

// Add a child to parentItem through the MenuItems API. This should make the chevron visible.
parentItem.MenuItems.Add(new NavigationViewItem() { Content = "Child 3" });
Content.UpdateLayout();

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

// Remove all children of parentItem. This should collapse the chevron
parentItem.MenuItems.Clear();
Content.UpdateLayout();

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

}
}