Implements Carousel control and sample carousel page #712

Mimetis · 2016-12-13T16:58:08Z

Implementing a first version of the Carousel control.

dnfclas · 2016-12-13T16:58:11Z

Hi @Mimetis, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-12-13T17:03:03Z

@Mimetis, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

IbraheemOsama · 2017-01-25T11:20:48Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml.cs

+namespace Microsoft.Toolkit.Uwp.SampleApp.SamplePages
+{
+    /// <summary>
+    /// A page that shows how to use the DropShadowPanel control.


This shouldn't be DropShadowPanel control :)

ah ah correct :) Copy / Paste "is not" a friend !!

IbraheemOsama · 2017-01-25T11:22:18Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml

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


you need to use XamlStyler in Xaml pages

IbraheemOsama · 2017-01-25T11:23:22Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml

+                        TransitionDuration="{Binding TransitionDuration.Value, Mode=OneWay}" 
+                        Depth="{Binding Depth.Value, Mode=OneWay}" 
+                        MaxVisibleItems="{Binding MaxVisibleItems.Value, Mode=OneWay}"
+                        x:Name="carouselControl"


The name Should start with Capital letter CarouselControl

IbraheemOsama · 2017-01-25T11:26:23Z

Why I can't move using keyboard ? I guess it will be helpful as it was the first thing I do when I browse images on any slider is to move using keyboard arrows.
What do you think ?

IbraheemOsama · 2017-01-25T11:30:51Z

When I change the selected item using touch or mouse, the SelectedIndex slider in the properties Window became useless as when manipulating it nothing happens

IbraheemOsama · 2017-01-25T11:31:26Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.bind

+      mc:Ignorable="d">
+
+
+


Why all this spaces ?

Thx, I have made the corrections

IbraheemOsama · 2017-01-25T11:33:15Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml.cs

+using Microsoft.Toolkit.Uwp.SampleApp.Models;
+using System;
+using System.Collections.ObjectModel;
+using System.Linq;


The using statements need to be rearranged and the non used need to be removed.

IbraheemOsama · 2017-01-25T11:34:14Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml.cs

+            datas.Add(new CarouselData { BitmapImage = new BitmapImage(new Uri("ms-appx:///Assets/Photos/NovemberHikeWaterfall.png", UriKind.Absolute)), Title = "14" });
+            datas.Add(new CarouselData { BitmapImage = new BitmapImage(new Uri("ms-appx:///Assets/Photos/Owl.png", UriKind.Absolute)), Title = "15" });
+
+            this.Datas = datas;


Please remove this keyword as it is useless.

Well, to be honest, using this or not is a "coding style" and I use "this" every time I can, to make my code the most readable as possible.
I know we can have a long long debate about this.
But anyway, is adding "this" or not, Something so important in this case ? :)

It is important as it is part of our guideline :) we need to keep the same coding style for the entire project

Ok, I will delete all the "this" keywords if they're not necessary :)

IbraheemOsama · 2017-01-25T11:42:00Z

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

+using System.Collections;
+using System.Collections.Generic;
+using System.Collections.Specialized;
+using System.Linq;


Please remove the unused namespaces

IbraheemOsama · 2017-01-25T11:44:28Z

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

+{
+    public class Carousel : Canvas
+    {
+        private double initialOffset;


This file is not using the Standard library rules for documentations and naming conventions

The last version has changed, can you take a look on it ?

IbraheemOsama · 2017-02-11T05:16:03Z

@Mimetis up ?

… into dev

lukasf · 2017-02-15T20:56:38Z

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

+
+            if (this.SelectedIndex == index)
+            {
+                this.SelectedIndex = this.SelectedIndex == 0 ? 0 : this.SelectedIndex - 1;


If the last item is removed, SelectedIndex should become -1. Here it would end up with 0.

…control, new Orientation Property, Performance increase.

deltakosh · 2017-03-01T22:36:55Z

Hey @Mimetis,do you think you can implement the requested changes? Thank you :)

skendrot · 2017-03-01T22:44:33Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.bind

+
+    <Border Margin="0">
+
+      <controls:Carousel ItemsSource="{Binding Datas}" Background="{StaticResource Brush-Grey-05}"


Make sure xaml in the bind file matches the xaml in the actual page

skendrot · 2017-03-01T22:45:21Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.bind

+        <controls:Carousel.ItemTemplate>
+          <DataTemplate>
+            <Grid >
+              <Grid.RowDefinitions>


No need for the Rows or even the Grid itself. Template can just be the Image

Right, corrected

skendrot · 2017-03-01T22:46:00Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml

@@ -0,0 +1,52 @@
+<Page x:Class="Microsoft.Toolkit.Uwp.SampleApp.SamplePages.Carousel"


Class name should have the 'Page' suffix

Right, corrected

skendrot · 2017-03-01T22:47:17Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml

+                                <RowDefinition Height="Auto" />
+                                <RowDefinition Height="Auto" />
+                            </Grid.RowDefinitions>
+                            <Image Width="200"


Make sure all xaml has gone through the xaml styler: https://github.com/Microsoft/UWPCommunityToolkit/blob/master/contributing.md#quality-insurance-for-pull-requests-for-xaml-controls

Right, corrected

skendrot · 2017-03-01T22:49:00Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Carousel/Carousel.xaml.cs

+                DataContext = propertyDesc.Expando;
+            }
+
+            var datas = new ObservableCollection<CarouselData>();


Images can be obtained from the PhotosDataSource class.

carouselControl.ItemsSource = await new Data.PhotosDataSource().GetItemsAsync()

All of the images were actually changed from pngs to jpgs recently

Thx for the tips, I changed the way sample work :)

skendrot · 2017-03-01T23:41:41Z

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

+            this.ManipulationMode = ManipulationModes.All;
+            this.IsHitTestVisible = true;
+
+            this.SubscribePropertyChanged("ItemsSource", OnCarouselPropertyChanged);


Use RegisterPropertyChangedCallback method to handle the ItemsSource changing, or listen to the Items.VectorChanged event to know when items have been added/removed/changed

Right, corrected

skendrot · 2017-03-01T23:45:23Z

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

+        {
+            base.PrepareContainerForItemOverride(element, item);
+
+            if (this.ItemTemplate == null)


Why not create a new CarouselItem to handle displaying the content and handle the transform? Then each CarouselItem could have the dependency properties required for render transform

I don't want to create a specialized item (CarouselItem) for each items.

Any reason why? I think it could simplify some things

Easier for the developer to submit an ItemTemplate based on a ContentControl than a CarouselItem, in my opinion :)

How so? The fact that a ListView uses a ListViewItem does not change how a user defines an ItemTemplate

Oh Yes, my bad you're right on this point.
Anyway, adding a CarouselItem implies adding a new control class and a new style.
I want to have something the most simpliest.
Adding a CarouselItem is, in my point of view, a good thing if we need events and so on on each items.

If the transform/animation stuff was to really be improved (in a future version), it should be changed to use Composition. As far as I understand, Composition does not work well with normal DPs, so introducing CarouselItems now and controlling them through DPs might go into the wrong direction. Just my 2 cents...

skendrot · 2017-03-01T23:47:48Z

Microsoft.Toolkit.Uwp.UI.Controls/Carousel/CarouselPanel.cs

+
+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    public class CarouselPanel : Panel


Any reason the functionality of this class is not in the Carousel control itself?
What happens if I use a CarouselPanel directly in my xaml? What happens if I use it as the panel for a ListView?

It won't work.
This CarouselPanel is needed because you want an ItemsControl.
That's why, when you create an object inheriting from ItemsControl, if you follow the guidelines and how it's implemented in object like ListView, you should create an ItemsPanel.
For Carousel , the problem is that the only ItemsTemplate that works is the CarouselPanel.
And in the other side, the only ItemsControl that could work with a CarouselPanel is the Carousel control.
Maybe in a next release, we could try to resolved this design problem :)

This is my understanding as well. On top of this, if you want to layout items, Panel is the way to go. However, Panel can't be focused and can't get keyboard/controller events, so you need a Control (such as the ItemsControl). I ran into the same "issue" in my implementation, and I believe this is by design

skendrot · 2017-03-01T23:49:04Z

Microsoft.Toolkit.Uwp.UI.Controls/Carousel/CarouselPanel.cs

+                    continue;
+                }
+
+                this.Carousel.SelectedIndex = i;


No guarantee that the panel will be placed within a Carousel

I Change the Carousel getter to raise an error if we use this panel in other ItemsControl

skendrot · 2017-03-01T23:54:27Z

Microsoft.Toolkit.Uwp.UI.Controls/Carousel/CarouselPanel.cs

+        {
+            var position = e.GetPosition(this);
+
+            for (int i = 0; i < this.Children.Count; i++)


If there was a CarouselItem, it could listen to tap events and set itself as the selected item. The Carousel could listen to the CarouselItem changing this and set it's own property.

Maybe.
Don't remember exactly why I made it like this, but in my opinion:

I don't want CarouselItem, to let more flexibility to developers

I think I made it like that because of Virtualization (less events to subscribe)

skendrot · 2017-03-02T00:02:53Z

Started a review, but after reviewing the CarouselPanel I stopped to think, "Can this be a ListViewBase?" Is there any reason not to make this a ListViewBase control? It's possible that the manipulation code can be replaced by using a ScrollViewer and setting the Vertical and HorizontalSnapPointsType.
Not sure if placement would work correctly, but maybe having a CarouselItem could help with that.

Thoughts?

Mimetis · 2017-03-02T19:00:45Z

I think ListViewBase raise too many events and set to many properties in my opinion.
I prefer to let the developer used it as an ItemsControl :)
To keep the control as simple as possible I think ItemsControls is enough ?

deltakosh · 2017-03-13T15:59:33Z

@nmetulev @IbraheemOsama Do you mind guys updating your reviews based on latest commits?
We will close 1.4 on 3/21

deltakosh · 2017-03-13T21:18:49Z

Microsoft.Toolkit.Uwp.SampleApp/App.xaml

@@ -68,11 +68,17 @@
                            <WebView Name="webView"
                                     Grid.Row="0"
                                     Visibility="Collapsed" />
-                            <Button x:Name="PrintButton"


deltakosh · 2017-03-13T21:19:29Z

@nmetulev your last commit is suspicious :)

nmetulev · 2017-03-13T21:36:43Z

Should be good now @deltakosh

@Mimetis, I updated the focus to work for Xbox better and added the automation property. However, working on the automation property I realized that we should change how focus works on the control so it focuses on each item instead of the control itself. Could you take a look at it and see what it would take to do that?

For me, that and the sudden jump to an item on inertial scrolling are the last two things that should be fixed.

deltakosh · 2017-03-14T20:17:09Z

Ping @Mimetis
We plan to close on 3/21 and I would love to merge this PR before

Mimetis · 2017-03-15T10:03:15Z

I'm currently working on the "sudden jump" behavior. But it's ... complicated :)
And the automation focuses is in my backlog

deltakosh · 2017-03-15T17:34:04Z

Do you think you can make it before 3/21?

Mimetis · 2017-03-15T22:08:51Z

I really dont know.
I tried severals solutions an no one is correct.
For instance it's (as far as I know) nearly impossible to modify inertia or delta translation during a Manipulation, that's why we use an animation.

And for the tab control, I have to make some implementations, to see if the behavior is correct.

So, for now, if those requests are mandatory, the response is No :)

For instance the control is actually working pretty well, maybe we could schedule these improvements for a next version ?

nmetulev · 2017-03-16T04:34:29Z

Just added code to change the tabstop to be on the items and use arrow focus. I'll take a look at the inertia if I have time tomorrow; otherwise I'll let you know.

Mimetis · 2017-03-16T11:59:11Z

Thx @nmetulev I just tried your solution about tabstop and it's working pretty well. Nice job !
About inertia and the sudden jump, let me know if you have any idea :)

deltakosh · 2017-03-16T20:46:11Z

We will check for the sudden jump. can you create the documentation?

nmetulev · 2017-03-17T04:49:24Z

I spent a bit of time investigating the sudden jump, and tried to resolve it by canceling the inertia when detected and running the animation to the closes item where the inertia would have ended (by calculating the speed and deceleration of the inertia). It seems to be promising, but it still has some issues which I don't think I can resolve this week.

My proposal is to merge the PR as soon as the docs are added, and we open a bug for this issue?

I pushed my work so far here. @Mimetis , could you take a look and see if this make sense? You know the code much better, and I'm hoping my experimentation might give you an idea :)

…into dev

deltakosh · 2017-03-17T15:27:30Z

perfectly fine with that. I'll merge as soon as we have a doc

Mimetis · 2017-03-19T19:22:59Z

Thx @nmetulev for your awesome work !
I'had already test a solution like yours, based on the Velocities and Inertia.
The problem now is that we don't have inertia behavior anymore and the animation, with gesture manipulation, is not as smooth as it was before.
An other problem, is the behavior when you make a really small manipulation. Try to just apply a minimal animation of 2 ou 3 pixels to left or right, you will see the item "jump to the next".

It's an interesting work, and I think we should work on it more if we want to release something based on the inertia interruption.

I will work right now on the documentation, since this problem won't affect the documentation, but we have to decide what kind of manipulation behavior we want

deltakosh · 2017-03-20T15:55:40Z

That's what I call a doc:)

deltakosh · 2017-03-20T15:57:36Z

I'm merging now. If someone found something wrong please open an issue

deltakosh · 2017-03-20T15:57:49Z

Congrats @Mimetis !

Implements Carousel control and sample carousel page

d058391

dnfclas added the cla-required label Dec 13, 2016

dnfclas added cla-signed and removed cla-required labels Dec 13, 2016

adding the carousel header, using build.ps1 powershell script

4a047f3

deltakosh added the need documentation 📃 label Dec 14, 2016

deltakosh added this to the v1.4 milestone Jan 23, 2017

IbraheemOsama reviewed Jan 25, 2017

View reviewed changes

IbraheemOsama suggested changes Jan 25, 2017

View reviewed changes

nmetulev mentioned this pull request Feb 14, 2017

Carousel control #683

Closed

Merge branch 'master' of https://github.com/Mimetis/UWPCommunityToolkit…

ccc6eb2

… into dev

lukasf reviewed Feb 15, 2017

View reviewed changes

spertus added 3 commits February 25, 2017 17:24

New Carousel Control. Inherits from ItemsControl, Keyboard and mouse …

962b5b0

…control, new Orientation Property, Performance increase.

XamlStyler pase

d924275

Update Headers

ba54cf2

skendrot suggested changes Mar 1, 2017

View reviewed changes

nmetulev added 3 commits March 13, 2017 12:56

fixed conflicts

3ab35bd

updated directional navigation to work better for xbox

5f062f8

added AutomationProperty.Name

53e9abd

deltakosh reviewed Mar 13, 2017

View reviewed changes

fixed merge conflict yet again

84a8f64

nmetulev added 2 commits March 15, 2017 19:50

Merge branch 'dev' into Mimetis-dev

2ed36cc

updated focus management of carousel

649baef

playing with inertia

e2634ec

continuing inertia investigation

2603354

Mimetis added 2 commits March 17, 2017 09:07

Merge branch 'dev' of https://github.com/Microsoft/UWPCommunityToolkit …

41938a9

…into dev

Merge remote-tracking branch 'refs/remotes/upstream/Mimetis-dev'

41c4fd8

Mimetis added 2 commits March 20, 2017 16:26

Adding docs for Carousel control

b395692

Fixing typos

186e526

deltakosh merged commit f5cb104 into CommunityToolkit:dev Mar 20, 2017


		<Border Margin="0">

		<controls:Carousel ItemsSource="{Binding Datas}" Background="{StaticResource Brush-Grey-05}"

		@@ -0,0 +1,52 @@
		<Page x:Class="Microsoft.Toolkit.Uwp.SampleApp.SamplePages.Carousel"

Implements Carousel control and sample carousel page #712

Implements Carousel control and sample carousel page #712

Conversation

Mimetis commented Dec 13, 2016

dnfclas commented Dec 13, 2016

dnfclas commented Dec 13, 2016

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

IbraheemOsama commented Jan 25, 2017

IbraheemOsama commented Jan 25, 2017

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

IbraheemOsama Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama commented Feb 11, 2017

Choose a reason for hiding this comment

deltakosh commented Mar 1, 2017

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

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

Mimetis Mar 2, 2017 • edited Loading

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

Choose a reason for hiding this comment

skendrot commented Mar 2, 2017

Mimetis commented Mar 2, 2017 • edited Loading

deltakosh commented Mar 13, 2017

Choose a reason for hiding this comment

deltakosh commented Mar 13, 2017

nmetulev commented Mar 13, 2017

deltakosh commented Mar 14, 2017

Mimetis commented Mar 15, 2017

deltakosh commented Mar 15, 2017

Mimetis commented Mar 15, 2017

nmetulev commented Mar 16, 2017

Mimetis commented Mar 16, 2017

deltakosh commented Mar 16, 2017

nmetulev commented Mar 17, 2017

deltakosh commented Mar 17, 2017

Mimetis commented Mar 19, 2017

deltakosh commented Mar 20, 2017

deltakosh commented Mar 20, 2017

deltakosh commented Mar 20, 2017

IbraheemOsama Jan 25, 2017 •

edited

Loading

Mimetis Mar 2, 2017 •

edited

Loading

Mimetis commented Mar 2, 2017 •

edited

Loading