Extensions and controls to the right namespace and more #1297
Conversation
…lkit.Uwp.UI.Extensions
…soft.Toolkit.Uwp.UI.Extensions
|
ping @hermitdave My 2c: Instead of completely changing the namespace and introducing a hard breaking change, we should keep the old classes in the existing namespace (but deprecated) and encouraging developers to change over to the new namespace. Basically, we have both |
|
Introducing a second class should have happened in 1.5. for 2.0 make the breaking change. |
|
Yes we discussed this late in 1.5 cycle.. Only had a few hours to go |
There was a problem hiding this comment.
@edwinvandriel we discussed this around the time 1.5 was being released and agreed that we should leave the existing extensions as is.
So we need to do the following
- Mark existing extension classes WebViewExtensions, ListViewBaseExtensions and HyperlinkExtensions as obsolete
- Create a copy of existing classes and rename them to WebView, ListViewBase and Hyperlink with correct namespace. The new classes could be split into partial classes as you did.
- I would suggest doing the same with WrapPanel. Leave existing one as is and mark it as obsolete
- Create a new WrapPanel with correct namespace.
- Mark existing SurfaceDialTextboxHelper, TextBoxMask, TextBoxRegex as obsolete
- Create a new SurfaceDialTextbox, TextBoxMask and TextBoxRegex within extensions folder / namespace
|
@edwinvandriel old WrapPanel could / should just inherit from the one in correct namespace. The old one however does be obsolete. |
|
@hermitdave VS keeps complaining when I do that. The namespace 'Microsoft.Toolkit.Uwp.UI.Controls' already contains a definition for 'WrapPanel' Microsoft.Toolkit.Uwp.UI.Controls. In my last commit I changed all the other obsolete classes and make them inherit the right class or class in the right namespace. |
|
@edwinvandriel the old wrap panel was under controls.wrappanel namespace. new one under controls and old one should stay where it was under controls.wrappanel. |
|
@hermitdave yes I know but even when I do that VS starts complaining. I'll give it another try. |
|
let me try that locally |
|
@edwinvandriel okay leave it as is w.r.t WrapPanel |
|
@hermitdave change it back to the WrapPanel namespace? |
|
@edwinvandriel no it could be a breaking change |
| xmlns:d="http://schemas.microsoft.com/expression/blend/2008" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | ||
| xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" | ||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Extensions" |
There was a problem hiding this comment.
can we use xmlns:extensions please ?
| @@ -1,7 +1,7 @@ | |||
| <Page x:Class="Microsoft.Toolkit.Uwp.SampleApp.SamplePages.TextBoxMaskPage" | |||
| xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Extensions" | |||
There was a problem hiding this comment.
can we use xmlns:extensions please ?
| @@ -1,7 +1,7 @@ | |||
| <Page x:Class="Microsoft.Toolkit.Uwp.SampleApp.SamplePages.TextBoxMaskPage" | |||
| xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Extensions" | |||
There was a problem hiding this comment.
can we use xmlns:extensions please ?
| @@ -2,7 +2,7 @@ | |||
| xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:common="using:Microsoft.Toolkit.Uwp.SampleApp.Common" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Extensions" | |||
There was a problem hiding this comment.
can we use xmlns:extensions please ?
| @@ -2,7 +2,7 @@ | |||
| xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:common="using:Microsoft.Toolkit.Uwp.SampleApp.Common" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Extensions" | |||
There was a problem hiding this comment.
can we use xmlns:extensions please ?
| @@ -57,7 +58,7 @@ public static string GetMask(DependencyObject obj) | |||
| /// </summary> | |||
| /// <param name="obj">TextBox Control</param> | |||
| /// <param name="value">Mask Value</param> | |||
| public static void SetMask(DependencyObject obj, string value) | |||
| public static void SetMask(TextBox obj, string value) | |||
There was a problem hiding this comment.
Can you change the sample to ensure that we have use both x:Bind and Binding.. I have a feeling TextBox is going to bust x:Bind
| @@ -67,7 +68,7 @@ public static void SetMask(DependencyObject obj, string value) | |||
| /// </summary> | |||
| /// <param name="obj">TextBox control</param> | |||
| /// <returns>placeholder value</returns> | |||
| public static string GetPlaceHolder(DependencyObject obj) | |||
| public static string GetPlaceHolder(TextBox obj) | |||
There was a problem hiding this comment.
Can you change the sample to ensure that we have use both x:Bind and Binding.. I have a feeling TextBox is going to bust x:Bind
| @@ -77,7 +78,7 @@ public static string GetPlaceHolder(DependencyObject obj) | |||
| /// </summary> | |||
| /// <param name="obj">TextBox Control</param> | |||
| /// <param name="value">placeholder Value</param> | |||
| public static void SetPlaceHolder(DependencyObject obj, string value) | |||
| public static void SetPlaceHolder(TextBox obj, string value) | |||
There was a problem hiding this comment.
Can you change the sample to ensure that we have use both x:Bind and Binding.. I have a feeling TextBox is going to bust x:Bind
| @@ -87,7 +88,7 @@ public static void SetPlaceHolder(DependencyObject obj, string value) | |||
| /// </summary> | |||
| /// <param name="obj">TextBox control</param> | |||
| /// <returns>CustomMask value</returns> | |||
| public static string GetCustomMask(DependencyObject obj) | |||
| public static string GetCustomMask(TextBox obj) | |||
There was a problem hiding this comment.
Can you change the sample to ensure that we have use both x:Bind and Binding.. I have a feeling TextBox is going to bust x:Bind
| @@ -97,7 +98,7 @@ public static string GetCustomMask(DependencyObject obj) | |||
| /// </summary> | |||
| /// <param name="obj">TextBox Control</param> | |||
| /// <param name="value">CustomMask Value</param> | |||
| public static void SetCustomMask(DependencyObject obj, string value) | |||
| public static void SetCustomMask(TextBox obj, string value) | |||
There was a problem hiding this comment.
Can you change the sample to ensure that we have use both x:Bind and Binding.. I have a feeling TextBox is going to bust x:Bind
|
@hermitdave I've made the changes except the Sample for TextboxMask, I'll take a look at that later |
| @@ -3,7 +3,7 @@ | |||
| xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" | |||
| xmlns:d="http://schemas.microsoft.com/expression/blend/2008" | |||
| xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" | |||
| xmlns:wrapPanel="using:Microsoft.Toolkit.Uwp.UI.Controls.WrapPanel" | |||
| xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls.WrapPanel" | |||
|
@edwinvandriel much appreciate the quick turnaround |
…s break with x:Bind
| @@ -195,14 +195,14 @@ | |||
| "DocumentationUrl": "https://raw.githubusercontent.com/Microsoft/UWPCommunityToolkit/dev/docs/controls/TileControl.md" | |||
| }, | |||
| { | |||
| "Name": "SurfaceDialTextboxHelper", | |||
| "Type": "SurfaceDialTextboxHelperPage", | |||
| "Name": "SurfaceDialTextbox", | |||
There was a problem hiding this comment.
Should these be moved under Helpers, or maybe even their own Extensions page in the sample app? ping @hermitdave
readme.md
Outdated
| @@ -110,7 +110,7 @@ Once you search you should see a list similar to the one below (versions may be | |||
| * [ViewExtensions](http://www.uwpcommunitytoolkit.com/en/master/helpers/ViewExtensions/) | |||
| * [VisualTreeExtensions](http://docs.uwpcommunitytoolkit.com/en/master/helpers/VisualTreeExtensions/) | |||
| * [WeakEventListener](http://docs.uwpcommunitytoolkit.com/en/master/helpers/WeakEventListener/) | |||
| * [WebViewExtensions](http://docs.uwpcommunitytoolkit.com/en/master/helpers/WebViewExtensions/) | |||
| * [WebView](http://docs.uwpcommunitytoolkit.com/en/master/helpers/WebView/) | |||
There was a problem hiding this comment.
In terms of documentation, should we still append Extensions (e.g. WebView Extensions). Or if we decide to move everything under an Extensions group, that would work as well.
|
@nmetulev @hermitdave I think it would be better to create a new category Extensions in the sample app and documentation and shift some stuff over. |
|
Extensions group and everything under that. |
|
@nmetulev @hermitdave made a lot of changes and think this will be good for review :-) |
|
@edwinvandriel why about x:Bind sample for textbox extensions? |
|
@hermitdave I've tested that and failed so I had to change the method parameter from TextBox to DependencyProperty. I do not understand why this happens. |
|
It's because of the auto generated code for x:Bind |
|
@hermitdave maybe it's better to keep the scope of this PR to put things in the right namespaces and I'll start another PR soon with changes to the samples where it's needed. |
|
@hermitdave, @edwinvandriel, up? |
|
@nmetulev sorry have been on holiday with bad connectivity :) let me double check |
| /// <summary> | ||
| /// Horizontal and Vertical stretch | ||
| /// </summary> | ||
| All |
There was a problem hiding this comment.
Should be use Both instead of All ?
Following the usage in
https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.stretchdirection
There was a problem hiding this comment.
It would be yet another breaking change, if we are ok with that? I'm all for having a consistent usage
….Xaml.Controls.StretchDirection
|
@nmetulev @hermitdave agree to change the enumeration from All to Both for consistency. I've pushed a commit with the change. |
Fixed issues from #983 (not all), #1140, #1175 for v2.0