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

Move NavigationView to Repeater #1683

Merged
merged 33 commits into from
Jan 13, 2020
Merged

Conversation

ojhad
Copy link
Contributor

@ojhad ojhad commented Nov 27, 2019

Also fixes ItemsRepeater bug #1671

Description

Replaced ListView with ItemsRepeater in NavigationView.

Motivation and Context

Issue #378
Issue #1477
NavigationView's dependency on ListView is 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. As more behavior gets added to the control, the more we have to fight with ListView in order to get desired behavior. Moving to ItemsRepeater will provide us more freedom in implementing future functionality as well as resolve some issues that the control currently has due to the usage of ListView.

How Has This Been Tested?

  • Added to and ran current test corpus
  • Tested with some customer apps such as XamlControlsGallery, Calculator, etc.

@ojhad ojhad requested review from licanhua, ranjeshj and 035 and removed request for 035 November 27, 2019 00:26
@licanhua
Copy link
Contributor

licanhua commented Dec 2, 2019

unsealed runtimeclass StackLayout : VirtualizingLayout

I don't know which one will be better.
NonVirtualizingStackLayout vs StackLayout with flag to disable virtualization.
We defined NonVirtualizingLayout, but we didn't use it. it's wierd #Resolved


Refers to: dev/Repeater/ItemsRepeater.idl:423 in 196b57d. [](commit_id = 196b57d, deletion_comment = False)

@StephenLPeters
Copy link
Contributor

StephenLPeters commented Dec 20, 2019

return position;

return a default value here #Resolved


Refers to: dev/NavigationView/NavigationViewItemAutomationPeer.cpp:192 in b9d33f5. [](commit_id = b9d33f5, deletion_comment = False)

@ojhad
Copy link
Contributor Author

ojhad commented Dec 21, 2019

if (const auto& nvi = container)

This check is verifying that the passed in parameter is not null.


In reply to: 568038366 [](ancestors = 568038366)


Refers to: dev/NavigationView/NavigationView.cpp:2065 in b9d33f5. [](commit_id = b9d33f5, deletion_comment = False)

@StephenLPeters
Copy link
Contributor

if (const auto& nvi = container)

its kindof a weird way to do that, why not just

if(container)
{
    if(auto const element = container.try_as<winrt::UIElement>())
    {

In reply to: 568130439 [](ancestors = 568130439,568038366)


Refers to: dev/NavigationView/NavigationView.cpp:2065 in b9d33f5. [](commit_id = b9d33f5, deletion_comment = False)

@ojhad
Copy link
Contributor Author

ojhad commented Jan 3, 2020

/azp run #Resolved

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 3, 2020

Azure Pipelines successfully started running 1 pipeline(s).

#Resolved

reverted ShouldPreserveNavigationViewRS4Behavior and ShouldPreserveNavigationViewRS3Behavior to old behavior. Old behavior was inccorectly determining OS version, however updating to correct behavior breaks tests, so reverting to old behavior for now
@ojhad
Copy link
Contributor Author

ojhad commented Jan 8, 2020

void NavigationView::UpdateTitleBarPadding()

I don't see any reason this could not be, but maybe because margins and height is being updated on multiple elements, to keep everything consistent? I think its best to leave this as is for now.


In reply to: 560544059 [](ancestors = 560544059)


Refers to: dev/NavigationView/NavigationView.cpp:3612 in 196b57d. [](commit_id = 196b57d, deletion_comment = False)

@ojhad
Copy link
Contributor Author

ojhad commented Jan 8, 2020

/azp run #Resolved

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 8, 2020

Azure Pipelines successfully started running 1 pipeline(s).

#Resolved

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 9, 2020

No pipelines are associated with this pull request.

#Resolved

1 similar comment
@azure-pipelines
Copy link

azure-pipelines bot commented Jan 9, 2020

No pipelines are associated with this pull request.

#Resolved

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

:shipit:

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

Successfully merging this pull request may close these issues.

5 participants