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

Proposal: Update NavigationView to use ItemsRepeater #378

Closed
YuliKl opened this issue Mar 4, 2019 · 2 comments
Closed

Proposal: Update NavigationView to use ItemsRepeater #378

YuliKl opened this issue Mar 4, 2019 · 2 comments
Labels
area-ItemsRepeater area-NavigationView NavView control team-Controls Issue for the Controls team

Comments

@YuliKl
Copy link

YuliKl commented Mar 4, 2019

Proposal: Update NavigationView to use ItemsRepeater

Summary

Instead of ListView, perhaps NavigationView should use ItemsRepeater as an implementation detail. This shouldn't have any direct impact on users but would help clean up the code.

Rationale

Currently NavigationView has a dependency on ListView in order to display the MenuItems. This becomes a drawback in places where we have to circumvent some of ListView's existing behaviors which results in complicated codepaths and is a way for bugs to be introduced.

Pros to moving to ItemsRepeater:

  • Greater control over selection behavior which will result in cleaner code. Currently item selection logic is tangled up with the ListView selection logic
    • Have to keep track of selection state in two different locations. We have to manage and sync selection when it is changed using NavigationView API or when it is changed in the ListView.
    • Have to deal with callbacks for scenarios where we don’t want selection on invocation
    • Moving items in and out of a listview also sometimes influences selection
  • Item-repeater is less policy-heavy. Less restrictions can potentially mean less problems when implementing other future functionality. This is an advantage for Top and Hierarchical Navigation as these features are not based on a single listview and require movement of items. This flexibility also gives us a greater control over functionality such as animations.
  • Circumvents ListView pre-rs5 bug which manifests in databinding scenarios when adding a container as the root of the datatemplate.

Cons to moving to ItemsRepeater:

  • Have to re-implement behavior that is handled by listview
    • Selection
    • Item Invocation
    • Container - Item mapping
    • Markup VS Databinding scenarios
      • since ItemsRepeater only takes an ItemsSource, we would have to handle the scenarios where either the menuitems are declared in MarkUp or the ItemsSource requires default container wrapping (NavigationViewItem)
    • AutomationPeer
    • ItemContainerStyle/ItemContainerStyleSelector
  • Scenarios where previous app implementations are dependent on ListView functionality (such as SelectorItem.IsSelected) would get broken. Would have to write supporting code for these issues.

Functional Requirements

Important Notes

Open Questions

@YuliKl YuliKl added the area-NavigationView NavView control label Mar 4, 2019
@jevansaks
Copy link
Member

@ojhad Can you add some details about the pro/cons of this? We talked about it offline and it seems righteous but is it worth the cost?

@marcelwgn
Copy link
Collaborator

I think this proposal was addressed with #1683 and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ItemsRepeater area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

4 participants