add StepFrequency to RangeSelector #1584

liakamp · 2017-10-31T21:43:29Z

PR Type

Improvement on RangeSelector

[ ] Bugfix
[x ] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[x ] Documentation content changes
[x ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

the rangeselector max and min values change according to the thumb position

PR Checklist

Please check if your PR fulfills the following requirements:

[ x] Tested code with current supported SDKs
Docs have been added / updated (for bug fixes / features)
Sample in sample app has been added / updated (for bug fixes / features)
Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

There is a DP called StepValue, and if it is set then the values and the thumbs move to the corresponding position of the next stepvalue.

Does this PR introduce a breaking change?

[ ] Yes
[x ] No

Other information

azchohfi · 2017-10-31T23:01:43Z

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

+                }
+
+                double newStep = Minimum + StepValue;
+                while (newStep < Maximum)


Can't this be done using math? MoveToStepValue could just use the rangeValue and play some math with it (mod and subtraction). A while loop seems unnecessary.

Agreed: if the only point in having the stepValues list is for the MoveToStepValue method, then it should be possible to do the same in that method just with simple math!

azchohfi

Docs and Sample app should also be updated.

pedrolamas · 2017-11-02T08:54:01Z

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

+                }
+
+                double newStep = Minimum + StepValue;
+                while (newStep < Maximum)


Agreed: if the only point in having the stepValues list is for the MoveToStepValue method, then it should be possible to do the same in that method just with simple math!

michael-hawker

I would suggest 'StepFrequency' as the name to align with the inbox Slider class.

michael-hawker · 2017-11-06T23:54:47Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/RangeSelector/RangeSelectorCode.bind

+                              Grid.Column="1"                              
+                              Minimum="@[Minimum:Slider:0:0-100]@"
+                              Maximum="@[Maximum:Slider:100:0-100]@"
+                              StepValue="@[StepValue:String:1]@"/>


Why is this type of String instead of DoubleSlider?

Should there be a slider to pick the step frequency instead of a text field? With minimum being the DefaultStepFrequency and maximum the Maximum value of the RangeSelector?

That makes sense to me :)

michael-hawker · 2017-11-06T23:58:06Z

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

@@ -606,6 +617,78 @@ private static void IsTouchOptimizedChangedCallback(DependencyObject d, Dependen
            rangeSelector.ArrangeForTouch();
        }

+        /// <summary>
+        /// Gets or sets the step value for range selector.


Should elaborate on the behavior change that setting this property makes.

michael-hawker · 2017-11-06T23:59:29Z

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

+        /// <value>
+        /// The value for step.
+        /// </value>
+        public double StepValue


Should this be StepFrequency to align with the naming for the inbox Slider class?

or maybe even TickFrequency, and then expose a collection for the Ticks?

Steps and Ticks are different. So, that'd expand the scope of the feature. Steps are the limits on the values, where Ticks are just visual. However, if you have both, then there's the SnapsTo property to define which one to use for actual value limiting. See Slider SnapsTo docs.

michael-hawker · 2017-11-07T00:41:06Z

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

+        {
+            if (rangeValue != Maximum && rangeValue != Minimum)
+            {
+                return rangeValue - Enumerable.Range((int)Minimum, (int)Maximum)


Can't this just be Minimum + ((int)((rangeValue - Minimum) / StepValue)) * StepValue?

or if there is a collection for Ticks exposed this should be totally replaced

Ticks are different than Steps, so with just steps, you'd still need to figure out the valid values.

michael-hawker · 2017-11-07T01:29:23Z

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

+                return;
+            }
+
+            if (rangeSelector.StepValue != 0)


This check is redundant as the function does it already.

michael-hawker · 2017-11-07T01:30:20Z

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

+
+        private void ResetStepValues()
+        {
+            if (StepValue != 0)


Would be good to compare to 0.0, but better to probably pull out as a constant of 'DefaultStepFrequency' so it's clear what we're checking for (and use that in the property initialization too).

michael-hawker · 2017-11-13T21:04:52Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/RangeSelector/RangeSelectorCode.bind

+                              Grid.Column="1"                              
+                              Minimum="@[Minimum:Slider:0:0-100]@"
+                              Maximum="@[Maximum:Slider:100:0-100]@"
+                              StepFrequency="@[StepFrequency:String:1]@"/>


Can you make this a DoubleSlider so that they can just slide to change the step value and not guess what to type in as a string?

E.g.
@[StepFrequency:DoubleSlider:1.0:0.25-10.0]@

yes this will be a good thing to add, it will make more sense.

michael-hawker · 2017-11-13T21:07:07Z

docs/controls/RangeSelector.md

@@ -48,6 +48,10 @@ This is because by default, the ScrollViewer will block the thumbs of the RangeS

 ```

+## StepValue


Need to rename this to match now too.

nmetulev · 2017-11-15T18:14:46Z

@liakamp, just FYI, to make sure this will be merged for 2.1, it will need to be merged today. Otherwise, it will go out with 2.2

liakamp · 2017-11-15T20:03:44Z

@nmetulev I was testing this today and I think I should also add a limit or reject step frequency values that are more than the Maximum of the RangeSelector, so I would like to spend a bit more time on this later this week. Maybe it is a better idea to be released in 2.2

nmetulev · 2017-11-15T20:42:23Z

Sounds good to me :)

nmetulev · 2018-01-19T03:47:13Z

@liakamp, up?

liakamp · 2018-01-22T06:25:59Z

I want feedback on the last commit where the StepFrequency resets to default if it is greater than Maximum. If you agree I'm done.

skendrot · 2018-01-22T16:35:24Z

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

        private const double Epsilon = 0.01;

+        private const double DefaultStepFrequency = 0.0;


This should default to 1

Private const should be declared at the top of the class

skendrot · 2018-01-22T16:39:35Z

docs/controls/RangeSelector.md

@@ -48,6 +48,10 @@ This is because by default, the ScrollViewer will block the thumbs of the RangeS

 ```

+## StepFrequency
+
+If you want to use the RangeSelector using a step value, there is a `StepValue` property to set the interval between the values on the RangeSelector. For example; if you set `StepValue` to 2, using Minimum at 0 and Maximum at 10, the range values you can set will be 0,2,4,6,8,10.


What is StepValue? Is that suppose to be StepFrequency?

yes, I missed that

skendrot · 2018-01-22T16:42:47Z

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

@@ -606,6 +619,79 @@ private static void IsTouchOptimizedChangedCallback(DependencyObject d, Dependen
            rangeSelector.ArrangeForTouch();
        }

+        /// <summary>
+        /// Gets or sets the step frequency, which is the interval between the values on the RangeSelector


I would suggest making this the same as the docs for Slider:

//
// Summary:
// Gets or sets the value part of a value range that steps should be created for.
//
// Returns:
// The value part of a value range that steps should be created for.

skendrot · 2018-01-22T16:49:43Z

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

+
+        private void ValidateStepFrequency()
+        {
+            if (StepFrequency > Maximum)


Should follow behavior of Slider StepFrequency and just be the value set by the user. Even if that value is greater than the max. When sliding we could use Math.Min(Maximum, StepFrequency)

liakamp · 2018-01-23T06:20:20Z

one more change left, modify for Math.Min(Maximum, StepFrequency) and maybe remove some checks because of this. will add it later today

… step

skendrot · 2018-01-24T16:46:28Z

I think we're almost there. This should behave the same as on the Slider. I am noticing differences between the two. These can be easily seen by adding a Slider to the sample page and changing the StepFrequency, the Maximum/Minimum

<TextBlock Grid.Column="0" 
            HorizontalAlignment="Left"
            VerticalAlignment="Center"
            Foreground="Black"
            Text="{Binding Value, ElementName=Slider, Converter={StaticResource StringFormatConverter}, ConverterParameter='{}{0:0.##}'}" />
<Slider x:Name="Slider"
          Grid.Column="1"
          Minimum="@[Minimum:Slider:0:0-100]@" 
          Maximum="@[Maximum:Slider:100:0-100]@" 
          StepFrequency="@[StepFrequency:DoubleSlider:1.0:0.25-10.0]@"/>

Differences noted:

Value does not change on Slider when StepFrequency changes
Slider has smooth movement even with large StepFrequency. Value will change as it reaches next value. Thumb will snap to position if released not on a StepFrequency

liakamp · 2018-01-24T19:39:31Z

Thanks for the code snippet, I will give it a try and get back to you with changes

nmetulev · 2018-02-12T23:05:59Z

ping @liakamp

nmetulev · 2018-02-14T05:46:46Z

FYI, 2.2 code freeze is in one week. Do you think you will be able to do the changes before then?

…p frequency

…ange

liakamp · 2018-02-19T21:59:32Z

I was preparing an unreasonably complicated solution for this, turns out all I had to do was change the thumb sync. Hope now its ready for next release.

nmetulev

This is great. The only concern I have is the default step frequency being 1.0 since it changes the default behavior and it would be considered a breaking change. You can probably just change the StepFrequency to Nan or 0.01

skendrot · 2018-02-19T23:51:32Z

I did request the value be set to 1 to match the slider. Rarely do users need .01 accuracy on a slider.

nmetulev · 2018-02-19T23:56:23Z

I agree it should be 1, but it's still changing the default behavior, don't you think? We could change it to 1 for 3.0 or we could wait for 3.0 and mark it as a minor breaking change

skendrot · 2018-02-20T15:36:14Z

Yea, it would be a change to the previous behavior, but would anyone really complain? 😃
Let's make it .01 by default and move it to 1 for 3.0

nmetulev · 2018-02-20T18:22:19Z

Added it to #1615 to change to 1.0 :)

liakamp · 2018-02-20T19:15:54Z

@nmetulev can we add also to remove the IsTouchOptimized DP in v3? It is DP and not default because it would cause a breaking change - see PR #964

nmetulev · 2018-02-20T20:05:34Z

@liakamp, added

Stale review

skendrot · 2018-02-20T22:52:42Z

We are almost there!!! It looks like the Slider rounds up/down to get the new value rather than if it reaches the value.
Example: Set StepFrequency to 10. If the current max value is 50, then when dragging to the right it should change to 60 when it passes halfway between 50 and 60. If you release at that point, it should snap to it's actual location, much like already happens

PS: Sorry to be that guy!

nmetulev · 2018-02-21T21:02:40Z

ping @skendrot

skendrot

Great job!

windowstoolkitbot · 2018-02-21T21:12:15Z

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1356, #1615

add StepValue to RangeSelector

e3c0e74

nmetulev added controls 🎛️ reviewers wanted labels Oct 31, 2017

nmetulev added this to the v2.1 milestone Oct 31, 2017

azchohfi reviewed Oct 31, 2017

View reviewed changes

azchohfi previously requested changes Oct 31, 2017

View reviewed changes

pedrolamas previously requested changes Nov 2, 2017

View reviewed changes

liakamp added 3 commits November 5, 2017 18:53

remove stepValues list, find closest step value using math

4b0dee7

add StepValue to sample page

7c71237

update docs with StepValue

f41b5b8

michael-hawker requested changes Nov 7, 2017

View reviewed changes

liakamp added 5 commits November 11, 2017 10:41

rename StepValue to StepFrequency

8c505f3

elaborate on the behavior changes of the StepFrequency property

1b01678

remove redundant check for StepFrequency value

185a7cd

simplify step calculation

8d1ca6e

add DefaultStepFrequency constant

3c8026d

michael-hawker previously requested changes Nov 13, 2017

View reviewed changes

rename stepvalue to stepfrequency in md file

c31a729

nmetulev modified the milestones: v2.1, v2.2 Nov 15, 2017

liakamp added 2 commits January 22, 2018 07:19

add DoubleSlider to select value for StepFrequency

9819b6f

reset StepFrequency to default if it exceeds Maximum

032f6bf

skendrot suggested changes Jan 22, 2018

View reviewed changes

liakamp added 2 commits January 23, 2018 06:50

modify md file for the rename

d04907d

move private constants to the top

4b03bc4

modify summary for StepFrequency

2ffe566

liakamp changed the title ~~add StepValue to RangeSelector~~ add StepFrequency to RangeSelector Jan 23, 2018

set DefaultStepFrequency to 1, modify some checks on moving values to…

5c3fbd9

… step

nmetulev removed the reviewers wanted label Feb 13, 2018

liakamp added 2 commits February 19, 2018 22:43

sync thumbs only on drag completed for smoother effect when using ste…

fac1edf

…p frequency

remove adjustment of RangeMax and RangeMin values on StepFrequency ch…

fbfa3df

…ange

nmetulev reviewed Feb 19, 2018

View reviewed changes

change DefaultStepFrequency to avoid breaking change

dd22940

nmetulev approved these changes Feb 20, 2018

View reviewed changes

move value to the nearest step value by rounding

ca8deab

skendrot approved these changes Feb 21, 2018

View reviewed changes

skendrot merged commit 73b63cf into CommunityToolkit:master Feb 21, 2018

		@@ -48,6 +48,10 @@ This is because by default, the ScrollViewer will block the thumbs of the RangeS

		```

		## StepValue

		private const double Epsilon = 0.01;

		private const double DefaultStepFrequency = 0.0;

add StepFrequency to RangeSelector #1584

add StepFrequency to RangeSelector #1584

Conversation

liakamp commented Oct 31, 2017 • edited Loading

PR Type

What is the current behavior?

PR Checklist

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azchohfi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-hawker 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

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

nmetulev commented Nov 15, 2017

liakamp commented Nov 15, 2017

nmetulev commented Nov 15, 2017

nmetulev commented Jan 19, 2018

liakamp commented Jan 22, 2018

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

liakamp commented Jan 23, 2018

skendrot commented Jan 24, 2018 • edited Loading

liakamp commented Jan 24, 2018

nmetulev commented Feb 12, 2018

nmetulev commented Feb 14, 2018

liakamp commented Feb 19, 2018

nmetulev left a comment

Choose a reason for hiding this comment

skendrot commented Feb 19, 2018

nmetulev commented Feb 19, 2018

skendrot commented Feb 20, 2018

nmetulev commented Feb 20, 2018

liakamp commented Feb 20, 2018

nmetulev commented Feb 20, 2018

skendrot commented Feb 20, 2018

nmetulev commented Feb 21, 2018 • edited Loading

skendrot left a comment

Choose a reason for hiding this comment

windowstoolkitbot commented Feb 21, 2018

liakamp commented Oct 31, 2017 •

edited

Loading

skendrot commented Jan 24, 2018 •

edited

Loading

nmetulev commented Feb 21, 2018 •

edited

Loading