Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Slider] Add Logarithmic scale #6611

Closed
wants to merge 1 commit into from
Closed

Conversation

idoco
Copy link

@idoco idoco commented Apr 15, 2017

I've found myself implementing a log scale slider for our project and thought that it might be useful for others as well. Logarithmic scale is a useful way to represent a wide range of values and reduce them to a more manageable range - Wikipedia Link.

I've implemented it as a new Boolean property, logScale. When logScale is set to true, the value of the slider will increase by order of magnitude along the axis of the slider.

jdttpjyams

There is some math involved, and it is open for discussion. My inspiration was this math stackexchange question, and the function that worked best for me was:
math
But since I'm not an expert in this area, I would love to hear other ideas for a reversible function to map between linear and log scale.

notes:

  • I've set a fixed value (12) as the curveFactor for the function. I didn't extract it as a prop because I didn't want to confuse the user with implementation details, but I might be wrong.

  • I wasn't sure how to test it (if at all) and didn't find other tests that validate relation between the slider position and it's value, any suggestions?

  • Since this is my first contribution to material-ui, I might have missed some guidelines or unexpected effects that my change might have, so please give your feedback 😄

  • PR has tests / docs demo, and is linted.

  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".

  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@mbrookes mbrookes changed the title [Slider] Feature suggestion - Logarithmic scale slider [Slider] Add Logarithmic scale Apr 15, 2017
@mbrookes
Copy link
Member

@idoco What's the advantage to this being integrated over converting a linear scale to a logarithmic scale in the parent component?

@idoco
Copy link
Author

idoco commented Apr 15, 2017

@mbrookes Point taken, but I will share my perspective.

While implementing this functionality as a wrapper component for our project, I found it to be a non-trivial task. My idea was to wrap a slider ranged between 0 and 1 and to map this range with a logarithmic function. Writing this wrapper lead me to the understanding that the slider component has mixed roles, it is responsible for both for the rendering (slider position, axis, etc.) and value logic (min, max, step, etc.). In order for my solution to be really generic I had to re-implement most of the value logic, and that feels redundant.

You can argue that the slider will be completely useful even if it ranged only between 0 to 1, and didn't encapsulate the 'value logic', this way anyone can implement it's own logic, but I think it is more useful when it controls value clamping and step size.

Maybe it is a niche feature (It's hard to say from my perspective), but I find it reasonable as a type of slider just as reversed or vertical sliders 😄

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 15, 2017

But since I'm not an expert in this area, I would love to hear other ideas for a reversible function to map between linear and log scale.

@idoco Oh sweet, some old memories 😄

math

What's the advantage to this being integrated over converting a linear scale to a logarithmic scale in the parent component?

@mbrookes Huge 👍 for that point.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 15, 2017

From my perspective, the only way going forward is to provide an Higher-order component that handles the new domain. But as you raised, we need the transformation function f as well as the inverse function f-1 so that f o f-1 = Id. That would make the separation of concern much more elegant.

@idoco What do you think?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 15, 2017

Actually, I don't even think that we need an HOC, a demo in the docs should be enough, I would expect it to only add two lines of logic regarding the new domain.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI component: slider This is the name of the generic UI component, not the React module! and removed PR: Needs Review labels Apr 15, 2017
@alitaheri
Copy link
Member

This might not be so bad to have. And a better approach would be providing a hook for the normalization logic. There are many more scales that can be applied. We can add a prop: denorm that takes a value ranging from 0 to 1, a min value and a max value, that runs it through an easing function and returns the result ranging from min to max. the default would be: (value - min) / (max - min) a linear transformation.

The only advantage is the stepping behavior that can take place after the transformation.

It will only need a few lines of code (granted that we rename the ambiguous getPercent function to linear. getPercent doesn't get a percentage it only normalizes the input 😑

defaultProps: { denorm: linear}

@idoco
Copy link
Author

idoco commented Apr 16, 2017

Thanks for the feedback,
@oliviertassinari A demo in the docs would have been very helpful for me if it existed when I first started to think about slider transformation. As I see it, the main downside to this approach, is that the wrapping component will need to re-implement some of the value logic - min/max clamping, step size calculations, etc.

@alitaheri I really like the idea of a hook for the normalization function. Combined with few examples it would give exactly what I was looking for. The problem I had with implementing it this way, was the need for the reverse function in setValueFromPosition. The only solution that I could think of, was that the user will have to supply two functions so that f o f-1 = Id (As @oliviertassinari mentioned), but that didn't seem very elegant. If there was a way to extract the reverse function from the hook, or tackle this problem from another direction, I think that a denorm hook would be the best solution.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 16, 2017

The min/max are already here to provided a parametrized affine function with the following benefit:

  1. Simpler definition for users that y = a.x +b where x is between 0 and 1.
  2. Provide accessibility aria-valuemax and aria-valuemin.

So what's the added value of backing a custom transformation function in the component? I guess that it's for the accessibility so we can simply get the min value by calling tranform(0) and the max value by calling transform(1).

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 16, 2017

but that didn't seem very elegant

@idoco From my perspective, it's elegant. You can't just extract a revert function. It's much simpler by providing it. That's already what we are doing for the affine transformation provided out of the box with the Slider (min/max).

@idoco
Copy link
Author

idoco commented Apr 18, 2017

Hi @oliviertassinari , I'm not sure I understood from you comments how to proceed. Would you want me to re-implement this functionally as two new slider props, normalize and denormalize, that can be overridden to control slider normalization and update the example accordingly? (This doesn't have to be the final implementation, just as a reference for the discussion)

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 18, 2017

@idoco I don't know what's the best path going forward, regarding providing an example, here is what it took me to implement your example, it's +10 -4. So I would keep it simple by adding an example to help future people with the same concern.
@alitaheri What do you think?

diff --git a/docs/src/app/components/pages/components/Slider/ExampleControlled.js b/docs/src/app/components/pages/components/Slider/ExampleControlled.js
index 4aef55a..13cab77 100644
--- a/docs/src/app/components/pages/components/Slider/ExampleControlled.js
+++ b/docs/src/app/components/pages/components/Slider/ExampleControlled.js
@@ -1,18 +1,26 @@
 import React, {Component} from 'react';
 import Slider from 'material-ui/Slider';

+function transform(value) {
+  return Math.round((Math.exp(12 * value) - 1) / (Math.exp(12) - 1) * Math.pow(10, 6));
+}
+
+function reverse(value) {
+  return (1 / 12) * Math.log(((Math.exp(12) - 1) * value * Math.pow(10, -6)) + 1);
+}
+
 /**
  * The slider bar can have a set minimum and maximum, and the value can be
  * obtained through the value parameter fired on an onChange event.
  */
 export default class SliderExampleControlled extends Component {
   state = {
-    firstSlider: 0.5,
+    firstSlider: 500,
     secondSlider: 50,
   };

   handleFirstSlider = (event, value) => {
-    this.setState({firstSlider: value});
+    this.setState({firstSlider: transform(value)});
   };

   handleSecondSlider = (event, value) => {
@@ -23,8 +31,7 @@ export default class SliderExampleControlled extends Component {
     return (
       <div>
         <Slider
-          defaultValue={0.5}
-          value={this.state.firstSlider}
+          value={reverse(this.state.firstSlider)}
           onChange={this.handleFirstSlider}
         />
         <p>

@idoco
Copy link
Author

idoco commented Apr 18, 2017

@oliviertassinari good example, I think it also highlight the benefits of separating log-scale transformation and min/max/step control. As I see it, in the current example the transform function control both and makes it somewhat harder to change min/max values (Instead of being supplied as props to the slider they are embed in the function). Same for step size, which defaults to 0.01 (around 100k jump for the last step) and will need to be calculated by the user as 1 / (max - min) or something like that.

@oliviertassinari
Copy link
Member

@idoco I could have changed the example to use a different min and max property. I don't see how things could be simplified. Do you want to add a new example with that log scale transformation? That would help other 😄 .

@oliviertassinari
Copy link
Member

@idoco I have been updating the documentation with #6672 taking your feedback into account (step and min/max value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants