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

Jump to bottom

Update string reliance on WinUI NavigationView #4131

Merged
merged 4 commits into from Jul 28, 2021
Merged

Update string reliance on WinUI NavigationView #4131

merged 4 commits into from Jul 28, 2021

Conversation

XAML-Knight
Copy link
Contributor

Fixes #4127

Remove reliance upon hard-coded string

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the current behavior?

Retrieves WinUI NavigationView by hard-coded string

What is the new behavior?

FindAscendant for NavigationView with WinUI namespace

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@XAML-Knight XAML-Knight added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ improvements ✨ labels Jul 26, 2021
@ghost
Copy link

ghost commented Jul 26, 2021

Thanks XAML-Knight for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@michael-hawker
Copy link
Member

Thanks @XAML-Knight, think there's other optimizations we can do now too as we can make the type on Line 50 an actual NavigationView which means we can remove all the reflection based shenanigans that are happening like in SetNavigationViewBackButtonState.

Rosuavio
Rosuavio approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

Works on the sample App. I also did a search for similar scenarios and did not find any.

@net-foundation-cla
Copy link

net-foundation-cla bot commented Jul 27, 2021 •
edited

CLA assistant check
All CLA requirements met.

@net-foundation-cla
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ XAML-Knight sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

Sergio0694
Sergio0694 approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Looks great!
Always feels good to see reflection code being removed! 🚀🚀🚀

Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs Outdated
@@ -199,7 +199,7 @@ private void OnLoaded(object sender, RoutedEventArgs e)
_frame.Navigating -= OnFrameNavigating;
}

_navigationView = this.FindAscendants().FirstOrDefault(p => p.GetType().FullName == "Microsoft.UI.Xaml.Controls.NavigationView");
_navigationView = this.FindAscendant<Microsoft.UI.Xaml.Controls.NavigationView>();
Copy link
Member

Choose a reason for hiding this comment

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

Love seeing the new visual tree extensions being useful! 🙌

@Sergio0694 Sergio0694 added the optimization ☄ Performance or memory usage improvements label Jul 27, 2021
michael-hawker
michael-hawker reviewed Jul 27, 2021
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs Outdated
_previousNavigationViewBackVisibilty = (int)visibleProperty.GetValue(_navigationView);
visibleProperty.SetValue(_navigationView, visible);
}
_previousNavigationViewBackVisibilty = (int)_navigationView.IsBackButtonVisible;
Copy link
Member

Choose a reason for hiding this comment

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

We can strongly type to the enum value now over the int here too. 🙂

michael-hawker
michael-hawker reviewed Jul 27, 2021
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs Outdated
@@ -47,7 +47,7 @@ public partial class ListDetailsView : ItemsControl
private ContentPresenter _detailsPresenter;
private VisualStateGroup _selectionStateGroup;
private Button _inlineBackButton;
private object _navigationView;
private Microsoft.UI.Xaml.Controls.NavigationView _navigationView;
Copy link
Member

Choose a reason for hiding this comment

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

We could add a using at the top as well so we don't have to copy the namespace around:

using NavigationView = Microsoft.UI.Xaml.Controls.NavigationView;

@michael-hawker michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Jul 27, 2021
@XAML-Knight XAML-Knight merged commit cbefac4 into CommunityToolkit:main Jul 28, 2021
@michael-hawker michael-hawker added this to the 7.1 milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker left review comments

@Rosuavio Rosuavio Rosuavio approved these changes

@Sergio0694 Sergio0694 Sergio0694 approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees
No one assigned
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ improvements ✨ next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. optimization ☄ Performance or memory usage improvements
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

ListDetailsView uses hardcoded string to find NavigationView
4 participants
@XAML-Knight @michael-hawker @Rosuavio @Sergio0694