ViewExtensions and sample #1002

hermitdave · 2017-03-03T13:46:27Z

Added TitleBarExtensions
Added StatusBarExtensions
Added ApplicationViewExtensions
Added documentation for ViewExtensions

Added TitleBarExtensions Added StatusBarExtensions Added ApplicationViewExtensions Added documentation for ViewExtensions

hermitdave · 2017-03-06T14:53:35Z

@timheuer any thoughts on this?

deltakosh · 2017-03-06T17:15:08Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsCode.bind

+      viewHelper:StatusBarExtensions.BackgroundColor="Blue"
+      viewHelper:StatusBarExtensions.BackgroundOpacity="0.8"
+      viewHelper:StatusBarExtensions.ForegroundColor="White"
+      viewHelper:StatusBarExtensions.IsVisible="True"


what about binding it to property page? This way we could see live change :)

deltakosh · 2017-03-06T17:15:17Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsPage.xaml.cs

+namespace Microsoft.Toolkit.Uwp.SampleApp.SamplePages
+{
+    /// <summary>
+    /// An empty page that can be used on its own or navigated to within a Frame.


not required ;)

deltakosh · 2017-03-06T17:16:09Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsPage.xaml.cs

+            var greyBrush03 = (Color)Application.Current.Resources["Grey-03"];
+            var greyBrush01 = (Color)Application.Current.Resources["Grey-01"];
+
+            TitleBarExtensions.SetButtonBackgroundColor(this, greyBrush03);


I was thinking about a kind of recorder: like TitleBarExtensions.SaveState / restoreState

deltakosh · 2017-03-06T17:20:41Z

Microsoft.Toolkit.Uwp.UI/Extensions/StatusBarExtensions.cs

+        /// </summary>
+        /// <param name="obj">The <see cref="DependencyObject"/> typically <see cref="Page"/></param>
+        /// <returns><see cref="Color"/></returns>
+        public static Color GetBackgroundColor(DependencyObject obj)


Question that applies to all other properties: Why not attached properties? They could be animated or bound then.

hermitdave · 2017-03-06T17:36:27Z

@deltakosh the issue there is that title bar / status bar do not have one to one with Page. There's only one instance and that would be the reason why WP7 status bar did not support binding.

deltakosh · 2017-03-06T17:40:39Z

Makes sense!

skendrot · 2017-03-06T17:46:22Z

Has this been tested with page navigation? I remember a WinRT app I wrote that had issues with page navigation and change titlebar/statusbar properties when navigating back and forth

hermitdave · 2017-03-06T18:11:53Z

@skendrot hence the sample page

skendrot · 2017-03-06T18:17:15Z

There is only one sample page. Has it been tested going to another page that does not want any of the properties set?

hermitdave · 2017-03-06T18:18:09Z

Shell and the sample

hermitdave · 2017-03-06T18:18:34Z

I can add it to every child it that helps

hermitdave · 2017-03-06T18:26:58Z

@skendrot design of underlying object is such that once set they remain until changed

Odonno · 2017-03-06T19:09:32Z

Microsoft.Toolkit.Uwp.UI/Extensions/TitleBarExtensions.cs

+        /// <summary>
+        /// Gets a value indicating whether TitleBar is supported or not.
+        /// </summary>
+        public static bool IsTitleBarSupported => Windows.Foundation.Metadata.ApiInformation.IsTypePresent("Windows.UI.ViewManagement.ApplicationViewTitleBar");


Should it be ApiInformation.IsTypePresent("Windows.UI.ViewManagement.ApplicationView"))?
Or maybe you missed a .?

https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.viewmanagement.applicationviewtitlebar.aspx

Pproperty and type don't need to match

@hermitdave Oh. I didn't know that. Thanks for the info.

Odonno · 2017-03-06T19:12:37Z

Microsoft.Toolkit.Uwp.UI/Extensions/TitleBarExtensions.cs

+                titleBar.InactiveForegroundColor = value;
+            }
+        }
+


If possible, could you add a property to update ExtendViewIntoTitleBar?

Ok. I am sorry. I checked again and it is the same class name TitleBar but it is from another namespace. You can find use it in using CoreApplication.GetCurrentView().TitleBar and the class name is CoreApplicationViewTitleBar. It has 5 properties but I generally only use the ExtendViewIntoTitleBar.

Odonno · 2017-03-06T19:15:31Z

Another idea. It could be also useful to handle some properties of SystemNavigationManager. I use it often in my apps, it could be a simple property of the Page.

SystemNavigationManager.GetForCurrentView().AppViewBackButtonVisibility = AppViewBackButtonVisibility.Collapsed;

hermitdave · 2017-03-06T20:57:58Z

@Odonno I have exposed 2 more properties..
@deltakosh still want me to tweak things ?

deltakosh · 2017-03-06T21:11:17Z

I still have several questions unanswered :)

hermitdave · 2017-03-06T21:11:54Z

@deltakosh I am sure you already have the branch and are tweaking stuff as we speak

Odonno · 2017-03-06T21:13:53Z

@hermitdave For me, it's really good. If you can add the new properties in the documentation, it will be perfect.

deltakosh · 2017-03-06T21:16:16Z

No I mean: I have some comments in your code (like the one about property page or the not required comments)

hermitdave · 2017-03-06T21:17:53Z

@deltakosh so you want me to remove the bind page and the sample page ? or just the bind ?

deltakosh · 2017-03-06T21:18:58Z

i want to use the integrated mechanism with IsVisible=[Boolean:True]
And remove the unnecessary comments

Set xaml binding

skendrot

Can we name the classes "TitleBar", "StatusBar", etc instead of "StatusBarExtensions", "TitleBarExtensions", etc. This does complicate things for code, but greatly simplifies for xaml.

Should the methods only accept a Page? Right now you could nest the properties into a button if you wanted which wouldn't make sense. Makes more sense to restrict the access to them to a Page

skendrot · 2017-03-08T15:03:20Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsPage.xaml.cs

+            TitleBarExtensions.SetForegroundColor(this, lightGreyBrush);
+        }
+
+        private void Button_Tapped(object sender, Windows.UI.Xaml.Input.TappedRoutedEventArgs e)


Where is this event created/subscribed to?

@skendrot vestigial code.

skendrot · 2017-03-08T15:04:23Z

Microsoft.Toolkit.Uwp.SampleApp/Shell.xaml

      xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
      xmlns:local="using:Microsoft.Toolkit.Uwp.SampleApp"
      xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
+      xmlns:viewHelper="using:Microsoft.Toolkit.Uwp.UI.Extensions"


can we change the namespace declaration to viewExtensions or just extensions. "viewHelper" doesn't make sense for the namespace

skendrot · 2017-03-08T15:04:45Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsCode.bind

+      xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
+      xmlns:local="using:Microsoft.Toolkit.Uwp.SampleApp.SamplePages"
+      xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
+      xmlns:viewHelper="using:Microsoft.Toolkit.Uwp.UI.Extensions"


can we change the namespace declaration to viewExtensions or just extensions. "viewHelper" doesn't make sense for the namespace

hermitdave · 2017-03-08T15:56:20Z

@skendrot being able to attach them makes sense but that requires attached properties. This is something @deltakosh is keen on though. I might close this PR and someone else can pick it up.

skendrot · 2017-03-08T16:01:56Z

@hermitdave Are you referring to this?

Should the methods only accept a Page? Right now you could nest the properties into a button if you wanted which wouldn't make sense. Makes more sense to restrict the access to them to a Page

If so, I'm merely suggesting to change the method parameter you have from DependencyObject to Page.
Example:

public static class StatusBarExtensions
{
    public static Color GetBackgroundColor(Page page)
    {
        ....
    }
    public static void SetBackgroundColor(Page page, Color value)
    {
        ....
    }
}

hermitdave · 2017-03-08T16:06:40Z

@skendrot right you are.. I'll fix the PR if we keep it.

deltakosh · 2017-03-08T16:13:31Z

We should keep it. it is clearly useful

hermitdave · 2017-03-08T16:24:24Z

I meant we should create a new PR

deltakosh · 2017-03-08T16:59:53Z

why?

Odonno · 2017-03-08T18:47:52Z

@skendrot @hermitdave Yeah. Definitely. If we can remove the "Extensions" suffix in xaml, it could improve the readibility of the code. I mean, not necesary but if that's possible, we should do it.

deltakosh · 2017-03-08T19:14:35Z

We can merge this one and do a second one for the requested changes if you want @hermitdave :)

hermitdave · 2017-03-08T19:27:22Z

@deltakosh i thought you preferred all properties to be bindable

hermitdave · 2017-03-08T19:27:51Z

That was the only reason for suggesting this PR be closed

deltakosh · 2017-03-08T19:28:14Z

Oh no I was fine with your explanation.

hermitdave · 2017-03-08T19:32:27Z

I'll push changes tonight

hermitdave · 2017-03-08T21:10:22Z

@skendrot can you review the changes ?

skendrot · 2017-03-09T04:03:32Z

I like the new names. Were you going to change the methods to accept Page instead of DependencyObject?

hermitdave · 2017-03-09T06:28:59Z

@skendrot silly me.. Forgot

Odonno · 2017-03-09T11:09:51Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewExtensions/ViewExtensionsCode.bind

      extensions:StatusBar.BackgroundOpacity="0.8"
      extensions:StatusBar.ForegroundColor="White"
-      extensions:StatusBar.IsVisible="@[IsVisible:Bool:false]"
+      extensions:StatusBar.IsVisible="@[StatusBar IsVisible:Bool:false]"


@hermitdave Why that change? Does it work like that?

Maybe not!!!

hermitdave · 2017-03-09T11:48:12Z

@Odonno fixed that.. thanks.. should have tested it on mobile as well!!

Odonno · 2017-03-09T20:55:47Z

@hermitdave Yeah. Great job. I am eager to use it.

ViewExtensions and sample

a7ea337

Added TitleBarExtensions Added StatusBarExtensions Added ApplicationViewExtensions Added documentation for ViewExtensions

dnfclas added the cla-already-signed label Mar 3, 2017

Update headers

956f040

hermitdave mentioned this pull request Mar 3, 2017

ViewManagement extension methods #980

Closed

deltakosh suggested changes Mar 6, 2017

View reviewed changes

Odonno suggested changes Mar 6, 2017

View reviewed changes

Expose more properties

e40ec53

Odonno approved these changes Mar 6, 2017

View reviewed changes

Change IsVisible to dependency property.

ebd9212

Set xaml binding

skendrot reviewed Mar 8, 2017

View reviewed changes

deltakosh added this to the v1.4 milestone Mar 8, 2017

rename extension classes

d809155

Use Page rather than DependencyObject

6e85397

Odonno reviewed Mar 9, 2017

View reviewed changes

Revert sample bind and set IsVisible to false in Shell.xaml

1cdbcb7

deltakosh approved these changes Mar 9, 2017

View reviewed changes

hermitdave merged commit 057e7a2 into dev Mar 9, 2017

hermitdave deleted the ViewHelpers-HD branch March 9, 2017 18:04

ViewExtensions and sample #1002

ViewExtensions and sample #1002

Uh oh!

Conversation

hermitdave commented Mar 3, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

deltakosh commented Mar 6, 2017

Uh oh!

skendrot commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

skendrot commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Odonno Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Odonno commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

deltakosh commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

Odonno commented Mar 6, 2017

Uh oh!

deltakosh commented Mar 6, 2017

Uh oh!

hermitdave commented Mar 6, 2017

Uh oh!

deltakosh commented Mar 6, 2017

Uh oh!

skendrot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermitdave commented Mar 8, 2017

Uh oh!

skendrot commented Mar 8, 2017

Uh oh!

hermitdave commented Mar 8, 2017

Odonno Mar 6, 2017 •

edited

Loading

Odonno commented Mar 6, 2017 •

edited

Loading

skendrot left a comment •

edited

Loading

Odonno commented Mar 8, 2017 •

edited

Loading

Odonno Mar 9, 2017 •

edited

Loading

hermitdave Mar 9, 2017 •

edited

Loading