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

[Shell] expose CurrentShell #4626

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
5 participants
@StephaneDelcroix
Copy link
Member

StephaneDelcroix commented Dec 4, 2018

Description of Change

[Shell] expose CurrentShell

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@samhouts samhouts added this to In progress in Sprint 145 via automation Dec 4, 2018

@samhouts samhouts added this to In Review in vNext (Target 3.5.0) Dec 4, 2018

Sprint 145 automation moved this from In progress to Ready for Review (PRs) Dec 5, 2018

@hartez
Copy link
Member

hartez left a comment

No problems, just questions.

@@ -162,8 +162,6 @@ static void OnShellColorValueChanged(BindableObject bindable, object oldValue, o

static readonly BindablePropertyKey MenuItemsPropertyKey =
BindableProperty.CreateReadOnly(nameof(MenuItems), typeof(MenuItemCollection), typeof(Shell), null, defaultValueCreator: bo => new MenuItemCollection());

#region IShellController

This comment has been minimized.

@hartez

hartez Dec 6, 2018

Member

Why drop the regions?

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Dec 6, 2018

Member

for a few reasons:

  • I don't like them
  • they're not consistent with the rest of the code, or even the rest of the shell code
  • they enforce some weird code ordering (this one had a region for BP, then a region for interface implementation. those 2, for example, can collide)
  • they're out of date faster than code comments

now, I get why one might like them: being able to collapse region. I know a thing or two about where to place opening braces, that saves SO MUCH vertical space that collapsing is no longer required AT ALL.

This comment has been minimized.

@hartez

hartez Dec 6, 2018

Member

Fair enough. I didn't realize they weren't being applied consistently.

I know a thing or two about where to place opening braces, that saves SO MUCH vertical space that collapsing is no longer required AT ALL.

:)

Show resolved Hide resolved Xamarin.Forms.Core/Shell/Shell.cs Outdated

Sprint 145 automation moved this from Ready for Review (PRs) to In progress Dec 6, 2018

@hartez hartez self-assigned this Dec 6, 2018

Update Xamarin.Forms.Core/Shell/Shell.cs
Co-Authored-By: hartez <hartez@users.noreply.github.com>
@hartez

hartez approved these changes Dec 6, 2018

Sprint 145 automation moved this from In progress to Ready for Review (PRs) Dec 6, 2018

@samhouts samhouts moved this from In Review to In Progress in vNext (Target 3.5.0) Dec 6, 2018

@rmarinho rmarinho merged commit 133e627 into 3.5.0 Dec 7, 2018

2 checks passed

Xamarin Forms #PR-4626 - (2259033) succeeded
Details
license/cla All CLA requirements met.

vNext (Target 3.5.0) automation moved this from In Progress to Done Dec 7, 2018

Sprint 145 automation moved this from Ready for Review (PRs) to Done Dec 7, 2018

@StephaneDelcroix StephaneDelcroix deleted the fix_gh4625 branch Dec 19, 2018

@samhouts samhouts added this to the 3.5.0 milestone Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment