[Slider] Remove style-propable mixin and react-dom #3332

felipethome · 2016-02-14T04:00:58Z

This PR removes the style-propable mixin and react-dom from the slider source.

It, also, fixes the line:

const calcDisabledSpacing = this.props.disabled ? ` -${disabledGutter}px` : '';

There should be a space between '-' and '${disabledGutter}px', otherwise it will not be valid CSS.

Just to point it out, the following statement changed from:

style={prepareStyles(this.props.style)}

to:

style={prepareStyles(Object.assign({}, this.props.style))}

Since, the reference to this.props.style is kept outside the render() method would start to produce the warning from callOnce everytime we call the render method for styles that use this.props.style

[Slider] Fix css calc for calcDisabledSpacing [Slider] Fix css calc for calcDisabledSpacing [Slider] Copy this.props.style directly in the div that uses it

newoga · 2016-02-14T06:52:25Z

src/slider.jsx

@@ -199,7 +193,7 @@ const Slider = React.createClass({
  getStyles() {
    const fillGutter = this.getTheme().handleSize / 2;
    const disabledGutter = this.getTheme().trackSize + this.getTheme().handleSizeDisabled / 2;
-    const calcDisabledSpacing = this.props.disabled ? ` -${disabledGutter}px` : '';
+    const calcDisabledSpacing = this.props.disabled ? ` - ${disabledGutter}px` : '';


I wonder, when this is not disabled, does it matter if it's undefined vs ''. If not, we could write this as

this.props.disabled && `-{disabledGutter}px`

It actually matters because the result of:

this.props.disabled && ` - {disabledGutter}px`

when this.props.disabled is false will be false. So in the string concatenation that happens after:

width: `calc(${(this.state.percent * 100)}%${calcDisabledSpacing})`

we would have the following string for example: 'calc(75% - false)'
I could change the code to behave as expected for that kind of assignment, but I would prefer not, because the advantage of a ternary is that we have the possibility to have a variable with the same type always (like in a functional approach), in that case a string. While, in the other case sometimes we have false and sometimes a string.

Oh good catch! Thanks!

newoga · 2016-02-14T07:15:53Z

Good job @felipethome!

Just to point it out, the following statement changed from:
style={prepareStyles(this.props.style)}
to:
style={prepareStyles(Object.assign({}, this.props.style))}

Good catch, yes we've been careful to make sure to not prepareStyles directly from passed in props but to compute a new object from it. 👍

I left some comments that were unrelated to your changes that might improve code quality in other areas but so far this looks good.

I forget to ask before, can you also move the getStyles() outside of the class definition like this. We're trying to move away from imperative methods and trying to makes these getStyles() functions purely a result from props and state. Doing so you'll have to remove the getTheme() calls from the getStyles() method (after which you can also delete that method).

felipethome · 2016-02-14T20:51:47Z

Thanks @newoga . And I will gladly remove the getTheme() function. I want to do it for a long time now 😄 .

I have a question, the slider has a lot of styles, some of them can be removed and I will do it soon, but most of them can't. A lot of these styles will me merged depending of state and prop variables. Right now, most of this merge computations are made in the render() method, but two of these merges are made in the getStyles(). I would like to have all these merge computations that depend of state or props ideally in one place. What do you think? (Note: right now it is easier if I make getStyles() free of state and props)

felipethome · 2016-02-14T21:14:46Z

I just realize that what I asked is not a good thing for the code. So, we don't need to change that. I will do the changes we talked about and remove getStyles to outside the class as you said, thanks!

newoga · 2016-02-14T21:26:22Z

@felipethome Yeah give it a shot. The general patter we've been following is construct the style object in getStyles(), merge with custom styles provided by props in render. Eventually many of these components can be written as functional components (which is pretty much what getStyles() is, except that it's not returning JSX).

felipethome · 2016-02-14T23:09:13Z

@newoga can you give a look? I am sorry I removed the clearValue function, I thought it was dead code since it is not being used inside the component and it doesn't seem to be in the docs, but maybe is some legacy code so I put the declaration back to its place.'

Another thing, we are spreading all the props inside the root element:

const {...others} = this.props;
...
<div {...others} style={prepareStyles(Object.assign({}, this.props.style))}>

Should I fix that to spread only the ones left? I mean, the ones the component doesn't explicitly use.

Another thought, we call of "handle" the element that we drag, but it is really common to use the word handle in functions that are manipulating events, what do you think about change the name "handle" to "knob" or "thumb"?

newoga · 2016-02-15T01:08:11Z

Looking good!

@newoga can you give a look? I am sorry I removed the clearValue function, I thought it was dead code since it is not being used inside the component and it doesn't seem to be in the docs, but maybe is some legacy code so I put the declaration back to its place.'

Yeah, it's probably better to leave it in for now. That being said, we should consider deprecating those methods for the next release since this component is controlled with value and onChange.

Should I fix that to spread only the ones left? I mean, the ones the component doesn't explicitly use.

Yeah I think that's a good idea, let's add description, disabled, style etc. to the destructure. Though moving the {...other} destructure within the JSX to the left was a good idea too. 👍

Another thought, we call of "handle" the element that we drag, but it is really common to use the word handle in functions that are manipulating events, what do you think about change the name "handle" to "knob" or "thumb"?

Yeah I wondered the same thing, that's a good idea. Thumb seems like a good name based on the slider spec.

felipethome · 2016-02-15T01:16:10Z

Unfortunately, I think we will need to accept the name "handle", because there are variables in the muiTheme that use this name like: handleSize or handleFillColor, so if I change in the end we would end up with two names ("handle" and "thumb").

newoga · 2016-02-15T01:19:00Z

because there are variables in the muiTheme that use this name like: handleSize or handleFillColor

Yikes, yes you're right. That makes me said 😞. @alitaheri @oliviertassinari thoughts on this?

felipethome · 2016-02-15T01:22:58Z

It makes me sad too! Look this:
handleMouseDown(e) // handle here means handling the event
_onHandleMouseDown(e) // handle here means the element we drag
_dragHandler(e) // we also have the word handler to help

newoga · 2016-02-15T01:23:37Z

src/slider.jsx

+      overflow: 'visible',
+      outline: 'none',
+    },
+    handleWhenDisabled: {


I see what you mean with these:
handleWhenDisabled, handledWhenPercentZero,handleWhenPercentZeroAndDisabled`, etc. it would be nice if we could replace all of these wit ha single handle key so that the we didn't need Object.assign call below.

This can probably be solved by ternary for some keys or if there's too much, something like what we did with badge should work.

They were already in the code actually. I will try hard to change that, but it will take some time because there are a lot of cases that need to be handled (pun intended 😄)

Yeah I know! Not your fault. 😄

of cases that need to be handled (pun intended 😄)

😆

newoga · 2016-02-15T01:25:44Z

Yeah that's brutal. I really would like to fix that. Let me think about options...

felipethome · 2016-02-15T02:43:19Z

@newoga do you think the following code is too difficult to read?

    handle: {
        boxSizing: props.disabled ? 'content-box' : 'border-box',
        position: 'absolute',
        cursor: props.disabled ? 'not-allowed' : 'pointer',
        pointerEvents: 'inherit',
        top: '0%',
        left: state.percent === 0 ? '0%' : `${(state.percent * 100)}%`,
        zIndex: 1,
        margin: `${(slider.trackSize / 2)}px 0 0 0`,
        width: props.disabled ? slider.handleSizeDisabled :
          state.active ? slider.handleSizeActive : slider.handleSize,
        height: props.disabled ? slider.handleSizeDisabled :
          state.active ? slider.handleSizeActive : slider.handleSize,
        backgroundColor: props.disabled ? slider.trackColor :
          state.percent === 0 ? slider.handleFillColor : slider.selectionColor,
        backgroundClip: 'padding-box',
        borderRadius: '50%',
        borderStyle: 'solid',
        borderColor: (state.focused || state.hovered) ? slider.trackColorSelected : slider.handleColorZero,
        borderWidth: state.percent === 0 && !props.disabled ? `${slider.trackSize}px` : 0,
        transform: 'translate(-50%, -50%)',
        transition:
          `${Transitions.easeOut('450ms', 'background')}, ${
          Transitions.easeOut('450ms', 'border-color')}, ${
          Transitions.easeOut('450ms', 'width')}, ${
          Transitions.easeOut('450ms', 'height')}`,
        overflow: 'visible',
        outline: 'none',
    },

The slider can have a lot of different states: active, hovered, focused, disabled, percent zero. That is why I am thinking about ternaries instead of if-else generalizing all the states. Maybe the original way is more readable?

newoga · 2016-02-15T03:11:57Z

Wow, you're not kidding 😆 Yeah maybe there's just too much state here to use ternarys for. Alright, let's leave it alone for now. I think there might be a way to break it up a bit still but let's see if we can get this merged now since we got style-prop and reactdom out.

newoga · 2016-02-15T03:12:18Z

@oliviertassinari @alitaheri Can you take a look at this when you get a chance?

felipethome · 2016-02-15T03:22:17Z

That is a pity, but yes..would be too much for this PR. Can I at least move this piece of code:

{
    left: `${(percent * 100)}%`,
}

To inside of the getStyles method()? Because then we get a render() free of CSS properties.

Also, just something to point:

const sliderStyles = Object.assign({}, styles.root, this.props.style);
...
<div {...others} style={prepareStyles(Object.assign({}, this.props.style))}>
    <span>{this.props.description}</span>
    <span>{this.props.error}</span>
    <div
      style={prepareStyles(sliderStyles)}
      ...

The this.props.styles are being applied two times, and should be just in the root element, right? But we probably would break some pages out there changing that right now.

newoga · 2016-02-15T03:41:27Z

Can I at least move this piece of code:

Yeah definitely, that makes sense

The this.props.styles are being applied two times, and should be just in the root element, right? But we probably would break some pages out there changing that right now.

I didn't even catch that, that's a bug in my opinion. We ran into that with the AutoComplete component recently too. The style prop should really only be used for the root element. I want to fix this too...

Looking at the code more closely, I wonder if the sliderStyles div should really be the root, and managing the error text and description should be handled outside of this component. This component doesn't really offer much flexibility in customizing the style for the description and error and I wonder if that functionality should be moved to a wrapper component. That way this component could simplify too and really be just the slider (not the slider control including label, etc.). (Don't do this now, just brainstorming)

felipethome · 2016-02-15T04:22:13Z

It doesn't offer anything at all for error and description. I think the slider component is more complex than what it seems when you just think about it. The material design specification also doesn't help us here because it has a lot of details about the design of this component.

That way this component could simplify too and really be just the slider (not the slider control including label, etc.)

I agree and If we had a directory just for the slider component, we would be able to break the implementation into pieces like: handle/thumb, track, errors, root. And that would simplify a lot..

Another awesome thing, would be a test case. Unfortunately, this test case isn't simple to write because of the amount of properties the user can set (that is the price we pay for customization).

newoga · 2016-02-15T04:41:35Z

I agree and If we had a directory just for the slider component, we would be able to break the implementation into pieces like: handle/thumb, track, errors, root. And that would simplify a lot..

I agree, I think splitting this up is a good idea (and agree there is a lot more complexity to this than it seems, including functionality in the spec that we haven't implemented yet). Soon all the components will be put in directories (#3212) and we should be able to split it up. 😓 I wouldn't be totally opposed to starting to split of the thumb into a SliderThumb component from now.

felipethome · 2016-02-15T04:54:20Z

Soon all the components will be put in directories (#3212)

Awesome!

I wouldn't be totally opposed to starting to split of the thumb into a SliderThumb component from now.

I would gladly be part of the discussion of how to split it, once you decide is time to do it.

I added the last changes we talked about, now I think it is done.

newoga · 2016-02-15T05:45:18Z

Okay awesome @felipethome, let's get some feedback from the other guys and we'll see what next steps are. Thanks so much, this is a huge help 👍

oliviertassinari · 2016-02-15T11:04:00Z

@felipethome That looks good 👍.

Side note: I have noticed that autoPrefix.set(body.style, 'userSelect', value, this.state.muiTheme);
doesn't work correctly. That's kind of expected, the autoPrefix.set method is broken.

felipethome · 2016-02-15T16:15:20Z

Thanks @oliviertassinari ! The PR #3237 will solve this problem if you accept it.

newoga · 2016-02-15T17:33:59Z

@felipethome Could you squash when you get a chance?

@alitaheri If all is green for you, feel free to merge. We'll get the autoPrefix and other related improvements in future PRs.

alitaheri · 2016-02-15T17:44:14Z

Actually only squash f392585 The rest of the commits are good for the history 👍 I'll review in a moment 👍

alitaheri · 2016-02-15T21:10:44Z

Side note: I have noticed that autoPrefix.set(body.style, 'userSelect', value, this.state.muiTheme);
doesn't work correctly. That's kind of expected, the autoPrefix.set method is broken.

Let's continue this on the other PR.

@oliviertassinari Feel free to merge, this looks good to me 👍

[Slider] Remove style-propable mixin and react-dom

oliviertassinari · 2016-02-15T21:17:41Z

@felipethome Thanks! 🎉

alitaheri · 2016-02-15T21:19:22Z

Awesome, thanks a lot @felipethome 👍 👍 👍

newoga · 2016-02-15T22:06:52Z

Great job! @felipethome 😄

felipethome · 2016-02-15T22:18:20Z

Thank all of you too! 😄

[Slider] Remove style-propable mixin and react-dom

14f65cc

[Slider] Fix css calc for calcDisabledSpacing [Slider] Fix css calc for calcDisabledSpacing [Slider] Copy this.props.style directly in the div that uses it

mbrookes added the PR: Needs Review label Feb 14, 2016

newoga reviewed Feb 14, 2016
View reviewed changes

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 14, 2016

felipethome added 2 commits February 14, 2016 21:02

[Slider] Move getStyles to outside the component class

2aa64ef

[Slider] Put back the clearValue() declaration

f392585

newoga reviewed Feb 15, 2016
View reviewed changes

newoga added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 15, 2016

[Slider] Destructure properties in render() and move css to getStyles()

906b16a

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Feb 15, 2016

oliviertassinari added a commit that referenced this pull request Feb 15, 2016

Merge pull request #3332 from felipethome/slider-remove-mixin

b442dc0

[Slider] Remove style-propable mixin and react-dom

oliviertassinari merged commit b442dc0 into mui:master Feb 15, 2016

zannager added the component: slider This is the name of the generic UI component, not the React module! label Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] Remove style-propable mixin and react-dom #3332

[Slider] Remove style-propable mixin and react-dom #3332

felipethome commented Feb 14, 2016

newoga Feb 14, 2016

felipethome Feb 14, 2016

newoga Feb 14, 2016

newoga commented Feb 14, 2016

felipethome commented Feb 14, 2016

felipethome commented Feb 14, 2016

newoga commented Feb 14, 2016

felipethome commented Feb 14, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga Feb 15, 2016

felipethome Feb 15, 2016

newoga Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

alitaheri commented Feb 15, 2016

alitaheri commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016

alitaheri commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

[Slider] Remove style-propable mixin and react-dom #3332

[Slider] Remove style-propable mixin and react-dom #3332

Conversation

felipethome commented Feb 14, 2016

newoga Feb 14, 2016

Choose a reason for hiding this comment

felipethome Feb 14, 2016

Choose a reason for hiding this comment

newoga Feb 14, 2016

Choose a reason for hiding this comment

newoga commented Feb 14, 2016

felipethome commented Feb 14, 2016

felipethome commented Feb 14, 2016

newoga commented Feb 14, 2016

felipethome commented Feb 14, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga Feb 15, 2016

Choose a reason for hiding this comment

felipethome Feb 15, 2016

Choose a reason for hiding this comment

newoga Feb 15, 2016

Choose a reason for hiding this comment

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016

felipethome commented Feb 15, 2016

newoga commented Feb 15, 2016

alitaheri commented Feb 15, 2016

alitaheri commented Feb 15, 2016

oliviertassinari commented Feb 15, 2016

alitaheri commented Feb 15, 2016

newoga commented Feb 15, 2016

felipethome commented Feb 15, 2016