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 4 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
65 changes: 65 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,70 @@ 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)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;
}

}
}