feat(Expander): add ability to change expander orientation #1336

Odonno · 2017-07-24T11:18:27Z

Linked to #922 where @hawkerm requested Expander DIrection/Orientation.

Might also solve #923 because I needed to handle Expander Toggle Button orientation using a LayoutTransformer, the RenderTransform component does not render very well in this case. I reused the LayoutTransformer control from this blog post http://igrali.com/2012/09/17/layout-transform-in-windows-8-winrt-xaml/

skendrot · 2017-07-24T14:25:57Z

Did you take a look a look at the LayoutTransformControl from the WinRT XAML Toolkit by @xyzzer?

Odonno · 2017-07-24T14:33:56Z

@skendrot Seems very similar. One interesting thing is that this control handle PropertyChange of Value of Transform component (like the change on Angle of RotateTransform).

Should I add the dependency to this Library and use this control instead of the one I recreated?

skendrot

Quick review. Will need to do a more detailed review later

skendrot · 2017-07-24T14:27:37Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/Expander.Constants.cs

+        /// <summary>
+        /// Key of the VisualStateGroup that set orientation of the control
+        /// </summary>
+        public const string OrientationGroupStateContent = "OrientationStates";


Why are these public?

skendrot · 2017-07-24T14:28:55Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/ExpanderOrientation.cs

+
+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    public enum ExpanderOrientation


How about ExpandDirection

skendrot · 2017-07-24T14:29:32Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/ExpanderOrientation.cs

+        /// <summary>
+        /// Top
+        /// </summary>
+        Top,


Let's use the same values as WPF
Down,
Up,
Left,
Right

skendrot · 2017-07-24T14:30:19Z

Microsoft.Toolkit.Uwp.UI.Controls/LayoutTransformer/LayoutTransformer.cs

+            // Can't tab to LayoutTransformer
+            IsTabStop = false;
+
+#if SILVERLIGHT


We'll never be Silverlight

skendrot · 2017-07-24T14:33:34Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/Expander.cs

+                return;
+            }
+
+            if (Orientation == ExpanderOrientation.Left)


change to a switch statement

skendrot · 2017-07-24T14:34:11Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/Expander.cs

+            }
+
+            // Apply rotation on expander toggle button
+            layoutTransformer.ApplyLayoutTransform();


could be null if styled out

I don't understand. Null is checked before using the method.

hawkerm · 2017-07-24T16:22:59Z

Microsoft.Toolkit.Uwp.UI.Controls/LayoutTransformer/LayoutTransformer.cs

@@ -0,0 +1,537 @@
+// ******************************************************************
+// Copyright (c) Microsoft. All rights reserved.
+// This code is licensed under the MIT License (MIT).


Can we incorporate this code like this and re-license it as it's from the SO post?

@hawkerm Good question. I checked and the code in blog post is originally from Silverlight XAML, so it is already the property of Microsoft. So, I suppose we can.

hawkerm · 2017-07-24T16:25:10Z

If the other LayoutTransformControl behaves in the same manner, I'd suggest to use it with the clearer license term compatibility.

skendrot · 2017-07-24T16:43:31Z

Does the header need to rotate? The WPF Expander never did

Odonno · 2017-07-24T16:43:42Z

The two options are possible IMO. But clearly, we should not reinvent the wheel and reuse the WinRTXamlToolkit.

Odonno · 2017-07-24T16:45:33Z

@skendrot I need to rotate because the Header content should rotate if you want the header text to rotate. If there was only the arrow icon, we should not need it.

skendrot · 2017-07-24T16:48:33Z

if you want the header text to rotate

My question is why does that have to be the default? Why have it built in? If the dev wants the header text to rotate, they can pull in the WinRTXamlToolkit and restyle. I don't think this toolkit needs a LayoutTransform when an established toolkit already has it.

Odonno · 2017-07-24T17:06:18Z

@skendrot I now use the control from WinRTXAMLToolkit. It works exactly like before and I removed the LayoutTransform control from the UWP Toolkit.

skendrot · 2017-07-24T17:16:14Z

But should this toolkit have a dependency on WinRTXamlToolkit?
In short, I don't think we need to rotate the text. I would vote to remove this completely

Odonno · 2017-07-24T17:26:40Z

@skendrot If I don't do it, it will not look pretty. If left or right oriented, the header will take too much space. I don't think that's what we want.

Odonno · 2017-07-24T17:31:52Z

Without LayoutTransform :

skendrot · 2017-07-24T17:36:37Z

Would need to fix the stretch of the header and the direction of the arrow.
Here's the same on WPF.

Using the following XAML

<Expander Header="This is the header" Background="Blue" IsExpanded="True" ExpandDirection="Right">
    <Grid Background="Red">
        <TextBlock Text="This is the content"/>
    </Grid>
</Expander>

skendrot · 2017-07-24T17:38:54Z

Remember that more than text can go into the header. Could easily put text with images and other information into the header. You would not want this rotated.

Odonno · 2017-07-24T17:59:06Z

Because the Header contains the arrow, the arrow is also rotated thanks to the LayoutTransform.

In my opinion, since the Header is on the side (reading it vertically and not horizontally), it feels "normal" to me that all the content is rotated, even if it is not only text.

hawkerm · 2017-07-25T07:37:52Z

It does seem heavy to add a whole new dependency; not sure about copying it out for inclusion here, is that generally done?

From a UX perspective, I think most people expect the text to be rotated in these cases (that's the default I've seen for other libraries). But if we're trying to be WPF compatible, then we probably want to leave the behavior the same.

I think though if rotating is not the default and has to be done through restyling, it'd be good to have a sample that shows how to do that as it is a common usage.

skendrot · 2017-07-25T14:09:50Z

Yes. I agree that some sort of sample, either in the toolkit itself or via a blog post would help here.

nmetulev · 2017-08-06T20:01:55Z

@xyzzer, any concerns with moving the LayoutTransformControl to the toolkit?

xyzzer · 2017-08-07T19:39:24Z

Hi guys, sorry for ignoring this thread. Work's been busy and I've been ignoring emails from UWP Community Toolkit.

I totally think you should use LayoutTransformControl if it's been shown to work. I haven't heard complaints. Can't recall where I took it from, but I might have simply ported the code from Silverlight Toolkit rather than writing my own version, so the quality should be great, right? :)

Also - I recommend just copying the code, but this is a very subjective opinion. My opinion of WinRT XAML Toolkit itself has grown to be that people should totally use it for quickly sketching apps, prototypes, etc. and all the controls and helpers available at their fingertips, but as soon as you start working on an app you'd like to ship - you should strongly consider copying and pasting the code out of the toolkit to make sure you're not dragging along a few megabytes of dead code to download and load with your app. Yes, you'd lose the benefit of updates to the toolkit benefiting future builds of your app, but I don't actually update it that frequently anyways and so you're more likely to fix the bugs yourself if they're in your own source code. I considered breaking the toolkit apart more granularly so that every component would be its own NuGet package, but that turned out to be a very daunting task that I quickly dropped and so there are only a few NuGet packages with larger and more loosely related sets of components. For UWP Community Toolkit thus it would make sense to similarly - include a copy of the source, so that people can use it easily without dragging along hidden dependencies with different namespaces and also so that if they want to see how things work in that control - they only have to use at the UWP Community Toolkit source instead of spelunking a bunch of different GitHub repos.

nmetulev · 2017-08-08T22:33:11Z

Great insight. I think we are all in agreement at this point, @Odonno, what do you think?

xyzzer · 2017-08-08T23:01:39Z

Microsoft.Toolkit.Uwp.UI.Controls/Expander/Expander.cs

@@ -16,6 +16,7 @@
 using Windows.UI.Xaml.Controls;
 using Windows.UI.Xaml.Controls.Primitives;
 using Windows.UI.Xaml.Markup;
+using WinRTXamlToolkit.Controls;


Make sure to move it to a better namespace.

xyzzer

Please update the namespaces from WRTXT to UWPCT

Odonno · 2017-08-09T07:12:01Z

@nmetulev I'm fine with it. Like I said, both options are possible. I'll copy the code and import it in the Toolkit.

Odonno · 2017-08-12T10:56:12Z

@nmetulev @xyzzer Done.

xyzzer · 2017-08-12T18:42:24Z

Thanks!

…

On Sat, Aug 12, 2017 at 3:56 AM, David Bottiau ***@***.***> wrote: @nmetulev <https://github.com/nmetulev> @xyzzer <https://github.com/xyzzer> Done. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABrAsK8fd1BvAcngitX2tfyGlZI0fZ5iks5sXYTSgaJpZM4OhDOc> .

nmetulev

Could you also update the docs for the Expander to include the new property and add documentation for the LayoutTransformControl?

nmetulev · 2017-08-17T05:54:51Z

Microsoft.Toolkit.Uwp.UI.Controls/LayoutTransformControl/LayoutTransformControl.Properties.cs

+{
+    /// <summary>
+    /// Control that implements support for transformations as if applied by
+    /// LayoutTransform (which does not exist in Silverlight).


Should the bit about Silverlight be removed?

All comments about Silverlight? I can do it.

Yeah, unless you believe those are relevant?

nmetulev · 2017-08-17T05:56:21Z

Microsoft.Toolkit.Uwp.UI.Controls/LayoutTransformControl/LayoutTransformControl.cs

+{
+    /// <summary>
+    /// Control that implements support for transformations as if applied by
+    /// LayoutTransform (which does not exist in Silverlight).


xyzzer · 2017-08-17T17:02:03Z

But those are accurate! ;) FWIW, I think we should keep them to keep the code funny.

…

On Thu, Aug 17, 2017 at 8:59 AM, Nikola Metulev ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Microsoft.Toolkit.Uwp.UI.Controls/LayoutTransformControl/ LayoutTransformControl.Properties.cs <#1336 (comment)> : > +// IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, +// TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH +// THE CODE OR THE USE OR OTHER DEALINGS IN THE CODE. +// ****************************************************************** + +using System.Collections.Generic; +using Windows.UI.Xaml; +using Windows.UI.Xaml.Controls; +using Windows.UI.Xaml.Media; + +namespace Microsoft.Toolkit.Uwp.UI.Controls +{ + /// <summary> + /// Control that implements support for transformations as if applied by + /// LayoutTransform (which does not exist in Silverlight). Yeah, unless you believe those are relevant? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABrAsB_alExJb58v3zUficqvEmnMIKn4ks5sZGN8gaJpZM4OhDOc> .

feat(Expander): add ability to change expander orientation

5b4632c

Odonno added controls 🎛️ improvements ✨ labels Jul 24, 2017

Odonno requested review from nmetulev and skendrot July 24, 2017 11:18

dnfclas added the cla-already-signed label Jul 24, 2017

fix(LayoutTransformer): clean warnings

65e6ba6

skendrot reviewed Jul 24, 2017

View reviewed changes

refactor(Expander): rename ExpanderOrientation to ExpandDirection

4e5f3af

hawkerm reviewed Jul 24, 2017

View reviewed changes

refactor(Expander): reuse LayoutTransform control from WinRTXAMLToolkit

d7f4010

Odonno added this to the v2.0 milestone Jul 25, 2017

xyzzer reviewed Aug 8, 2017

View reviewed changes

refactor(Expander): copy LayoutTransformControl code inside the toolkit

3097986

nmetulev requested changes Aug 17, 2017

View reviewed changes

nmetulev added the need documentation 📃 label Aug 17, 2017

Odonno added 2 commits August 17, 2017 20:37

docs(LayoutTransformControl): remove Silverlight comments

de9aaf4

docs(Expander): add documentation about ExpandDirection property

e8e878b

nmetulev approved these changes Aug 20, 2017

View reviewed changes

nmetulev removed the need documentation 📃 label Aug 20, 2017

IbraheemOsama added the reviewers wanted label Aug 21, 2017

azchohfi self-requested a review August 21, 2017 21:56

azchohfi approved these changes Aug 21, 2017

View reviewed changes

azchohfi merged commit c2c9a04 into CommunityToolkit:dev Aug 21, 2017

Odonno deleted the expanderOrientation branch August 22, 2017 06:55

nmetulev mentioned this pull request Aug 22, 2017

Expander Should Have ExpandDirection Property #922

Closed

nmetulev mentioned this pull request Oct 17, 2017

Add LayoutTransform Helper #923

Closed

feat(Expander): add ability to change expander orientation #1336

feat(Expander): add ability to change expander orientation #1336

Conversation

Odonno commented Jul 24, 2017

skendrot commented Jul 24, 2017

Odonno commented Jul 24, 2017

skendrot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkerm commented Jul 24, 2017

skendrot commented Jul 24, 2017

Odonno commented Jul 24, 2017

Odonno commented Jul 24, 2017

skendrot commented Jul 24, 2017

Odonno commented Jul 24, 2017

skendrot commented Jul 24, 2017

Odonno commented Jul 24, 2017

Odonno commented Jul 24, 2017

skendrot commented Jul 24, 2017 • edited Loading

skendrot commented Jul 24, 2017

Odonno commented Jul 24, 2017

hawkerm commented Jul 25, 2017

skendrot commented Jul 25, 2017

nmetulev commented Aug 6, 2017

xyzzer commented Aug 7, 2017

nmetulev commented Aug 8, 2017

Choose a reason for hiding this comment

xyzzer left a comment

Choose a reason for hiding this comment

Odonno commented Aug 9, 2017

Odonno commented Aug 12, 2017

xyzzer commented Aug 12, 2017 via email

nmetulev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Odonno Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xyzzer commented Aug 17, 2017 via email

skendrot commented Jul 24, 2017 •

edited

Loading

Odonno Aug 17, 2017 •

edited

Loading