[Slider] Feature Custom Icon #12600

adeelibr · 2018-08-20T22:01:05Z

I have added a new prop called thumb to the Slider component, which you can use like this.

import React from 'react';
import PropTypes from 'prop-types';
import { withStyles } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';
import Slider from '@material-ui/lab/Slider';
import DeleteIcon from '@material-ui/icons/Delete';

const styles = {
  root: {
    width: 300,
  },
};

class SimpleSlider extends React.Component {
  state = {
    value: 50,
  };

  handleChange = (event, value) => {
    this.setState({ value });
  };

  render() {
    const { classes } = this.props;
    const { value } = this.state;

    return (
      <div className={classes.root}>
        <Typography id="label">Slider Image</Typography>
        <Slider
          value={value}
          aria-labelledby="label"
          onChange={this.handleChange}
          thumb={<img src="/static/images/cards/live-from-space.jpg" alt="" />}
        />
        <Typography id="label">Slider Simple</Typography>
        <Slider value={value} aria-labelledby="label" onChange={this.handleChange} />
        <Typography id="label">Slider Icon</Typography>
        <Slider
          value={value}
          aria-labelledby="label"
          onChange={this.handleChange}
          thumb={<DeleteIcon style={{ color: 'green' }} />}
        />
      </div>
    );
  }
}

SimpleSlider.propTypes = {
  classes: PropTypes.object.isRequired,
};

export default withStyles(styles)(SimpleSlider);

This looks something like this

This is a clean version of #12485 which got messed up, I kept on trying for some hours. but I guess I messed that pr more. Lesson learnt at my end.

adeelibr · 2018-08-20T22:13:22Z

@oliviertassinari I deleted the old PR, and created a new PR from scratch updated from upstream master, and I didn't run yarn docs:api but the build is still failing for CircleCI.

@eps1lon can you point me in the write direction?

mbrookes · 2018-08-20T22:15:09Z

I didn't run yarn docs:api

You need to in order to update the slider API docs

adeelibr · 2018-08-20T22:23:40Z

@mbrookes Thank you so so much for running 'yarn docs:api` apparently I am on a Windows machines and when I compile the markup build through it, it just gets messed up CRLF vs LF issue.

We had 2 long threads on this #12485 #12584

mbrookes · 2018-08-20T22:33:03Z

No worries. BTW, no need to close one PR and open another - you can force push your fixed branch.

Please could you make the customization examples a little more practical? The first looks like a glitch, and the third doesn't make much sense. Why would a slider have a trash-can icon for it's thumb? 😄

No need for the second example - we already know what it looks like when not customized.

It also needs a heading "Custom thumb"? Not sure it warrants a description.

adeelibr · 2018-08-20T22:51:52Z

I have added a better demo

Along with the description, as instructed.

Regarding the part

BTW, no need to close one PR and open another - you can force push your fixed branch.

I tried doing it, tried for like 2 hours but I think I ended up messing the PR more, so created a new one. I'll spend some time tomorrow learning how to git reset, git revert and git reflog 💯

eps1lon · 2018-08-21T05:30:46Z

Interactions are currently not very smooth. The Icon bounces up and down on activation:

This also happens with the mui icon.

Chrome Version 68.0.3440.106 (Official Build) (64-bit)

adeelibr · 2018-08-21T12:06:24Z

@eps1lon I have made changes to the thumb icon, is this the right approach? your suggestions.

adeelibr · 2018-08-21T14:18:36Z

Regarding the CircleCI pipeline for ci/circleci: test_browser am I doing something wrong, I saw it's log but it didn't make much sense to me.

eps1lon · 2018-08-21T14:20:25Z

packages/material-ui-lab/src/Slider/Slider.js

+      `${withIcon ? '' : classes.thumb}`,
+      commonClasses,
+      `${withIcon ? classes.thumbTransparent : ''}`,
+    );


You can leverage classNames implementation here by passing an object with the classnames as keys and a truthy value.

Why not apply classes.thumb always and classes.thumbIcon if an icon is passed and style accordingly. This would make more sense in my opinion. thumbTransparent just feels wrong. I'll always try to avoid naming my css classes after their look.

You can leverage classNames implementation here by passing an object with the classnames as keys and a truthy value.

Done, I have made the changes. :)

Why not apply classes.thumb always and classes.thumbIcon if an icon is passed and style accordingly. This would make more sense in my opinion. thumbTransparent just feels wrong. I'll always try to avoid naming my css classes after their look.

Regarding why I am not using thumbIcon is because I am using it for the Thumbnail itself, so instead of thumbTransparent I used thumbIconWrapper

Also I can not use the classes.thumb if there is a custom icon, because the recent issue you posted on active hovering state was because of this class.

How would it look if you overwrite the conflicting styles? It might be confusing to users that this class is not always applied to the thumb.

Understood, i'll do something about it.

I have enabled classes.thumb even when it is used with a thumb icon.

Also I just wanted to say thank you for taking the time out of your busy schedule, to review my code. This really means a lot. Thank you.

adeelibr · 2018-08-21T14:47:15Z

And now circleCI is failing again, because the docs needs to be updated using yarn docs:api.

mbrookes

It looks lime the position of the custom icon needs to be offset to the left by half its width. Also, there are no visible focus or click states.

mbrookes · 2018-08-21T16:03:54Z

packages/material-ui-lab/src/Slider/Slider.js

    const trackProperty = vertical ? 'height' : 'width';
    const thumbProperty = vertical ? 'top' : 'left';
    const inlineTrackBeforeStyles = { [trackProperty]: this.calculateTrackBeforeStyles(percent) };
    const inlineTrackAfterStyles = { [trackProperty]: this.calculateTrackAfterStyles(percent) };
    const inlineThumbStyles = { [thumbProperty]: `${percent}%` };

+    /** Start Thumbnail Icon Logic Here */


Could you change references to Thumbnail to something less confusing? a thumbnail and a (Material) thumb are two different things.

Should I change the prop thumb to sliderIcon would that be better?

thumbIcon?

thumbIcon sounds nice. Should this be the external prop too. e.g, right now it is only thumb.

<Slider {...otherProps} thumb={} />

Should I change this to thumbIcon too, along with inside the code as well.

And in Slider.js file
Instead of Thumbnail use thumbIcon as ThumbIcon because I need to clone the element and pass some props to it.

Done, I have made changes. Can you please review. Again thank you for taking your busy time out to review my code. This really means a lot.

mbrookes · 2018-08-21T16:04:48Z

docs/src/pages/lab/slider/CustomIconSlider.js

+          thumb={
+            <img
+              alt="slider thumb icon"
+              src="/static/images/cards/live-from-space.jpg"


As previously mentioned, using a large image shrunk to the size of a thumb makes it look like a glitch (circular or not).

Should I remove this image example?

With this image, then yes.

Okay, understood.

Or if it's okay can I use a 50x50 image of a circle, to use here as thumb icon? Instead of removing it. I'll need to add a new file in the static/ folder for this.

I ended up using another image, if that is not okay. Please do let me know. I'll just remove the image demo from the slider.

mbrookes · 2018-08-21T16:08:18Z

docs/src/pages/lab/slider/CustomIconSlider.js

+
+    return (
+      <div className={classes.root}>
+        <Typography id="label">Slider Image</Typography>


"Image thumb"

mbrookes · 2018-08-21T16:09:06Z

docs/src/pages/lab/slider/CustomIconSlider.js

+            />
+          }
+        />
+        <Typography id="label">Slider Icon</Typography>


"Icon thumb"

adeelibr · 2018-08-21T22:00:18Z

I have updated the demo with a better image of a circle.

mbrookes · 2018-08-22T13:48:50Z

Should I change this to thumbIcon too, along with inside the code as well.

Makes sense to me. @eps1lon?

eps1lon

Just one minor nitpick but this looks good now. Great work 👍 I can see focus states but no activated states. Maybe add some in the example?

I guess we do the ts definitions in another PR or is @adeelibr ok with adding a line in Slider.d.ts?

Edit:
To get CI working merge master to get #12611

packages/material-ui-lab/src/Slider/Slider.js

adeelibr · 2018-09-08T10:41:27Z

@eps1lon I think the tests passed successfully this time.

mbrookes · 2018-09-08T20:33:34Z

packages/material-ui-lab/src/Slider/Slider.js

@@ -122,6 +122,25 @@ export const styles = theme => {
        height: 17,
      },
    },
+    /* Class applied to the thumb element if custom thumb icon provided` . */


There seems to be a s stray back-tick and space before the full-stop.

adeelibr · 2018-09-09T16:58:01Z

@mbrookes since I updated the comment on style, the yarn docs:api command needs to be run again. I have an issue on windows. Can you please run the commands for this PR.

adeelibr · 2018-09-11T15:32:50Z

@oliviertassinari this PR has no conflicts any more, can this be merged now?

mbrookes · 2018-09-11T17:08:03Z

docs/src/pages/lab/slider/CustomIconSlider.js

+import { withStyles } from '@material-ui/core/styles';
+import Typography from '@material-ui/core/Typography';
+import Slider from '@material-ui/lab/Slider';
+import StarsIcon from '@material-ui/icons/Gamepad';


GamepadIcon?

Oops! Updating it :P

I have updated it to Lens icon, which is basically a filled circle.

Stale

mbrookes · 2018-09-11T17:34:05Z

docs/src/pages/lab/slider/CustomIconSlider.js

+
+    return (
+      <div className={classes.root}>
+        <Typography id="label">Image thumb</Typography>


The ids need to be unique.

added different id's

mbrookes · 2018-09-11T17:36:54Z

packages/material-ui-lab/src/Slider/Slider.js

+      '&$activated': {
+        width: 22,
+        height: 22,
+        transition: 'none',


The thumb should grow and shrink.

the thumb does grow and shrink.

As you can see in the demo, the icon does grow and shrink.

I had some bloated styling, which was redundant. I removed it.

That seems to have fixed it in Firefox

mbrookes · 2018-09-11T17:41:32Z

packages/material-ui-lab/src/Slider/Slider.js

    const trackProperty = vertical ? 'height' : 'width';
    const thumbProperty = vertical ? 'top' : 'left';
    const inlineTrackBeforeStyles = { [trackProperty]: this.calculateTrackBeforeStyles(percent) };
    const inlineTrackAfterStyles = { [trackProperty]: this.calculateTrackAfterStyles(percent) };
    const inlineThumbStyles = { [thumbProperty]: `${percent}%` };

+    /** Start Thumb Icon Logic Here */
+    const withIcon = !!thumbIcon;


Is it necessary to convert thumbIcon to an explicit boolean?

can you suggest me a better approach, i'd love to do it that way. (always learning 😅 )

If thumbIcon isn't provided, it will be undefined, so evaluate to false. You could still use !! for clarity if you prefer, but use it directly in the ternary and classNames, rather than assigning to a variable.

removed redundant !!

mbrookes · 2018-09-11T17:54:36Z

docs/src/pages/lab/slider/CustomIconSlider.js

+import { withStyles } from '@material-ui/core/styles';
+import Typography from '@material-ui/core/Typography';
+import Slider from '@material-ui/lab/Slider';
+import LensIcon from '@material-ui/icons/Lens';


Would LensOutlined work so as so as to be distinct from the default?

This is how that would look like?

I wondered about that. Not ideal...

What about "adjust"? Otherwise "favorite" with a step slider as a sort of rating slider.

Should it have a default background color? e.g white?

With a default white backgroundColor it would look like this.

That'll work (for the purpose of a demo)

For this I have to change the thumbIconWrapper in my slider.js file

thumbIconWrapper: { backgroundColor: '#fff', },

Or should i override it with classes prop in the demo for this?

I have added it in the demo, is that okay? 😅 @mbrookes

mbrookes · 2018-09-13T11:18:46Z

@eps1lon Could you have a last look please?

eps1lon

LGTM 👍

mbrookes · 2018-09-13T21:27:35Z

@adeelibr We got there in the end 😄 Thanks for working on it.

feature slider icon

979c0cd

[docs] Update Slider API docs

cac07cd

adeelibr added 3 commits August 21, 2018 03:44

updated demo

49b716f

added custom description

201177e

corrected heading

1efcbf3

ran prettier to format code

5933838

oliviertassinari added the package: lab Specific to @mui/lab label Aug 21, 2018

improvements on thumb icon while draggin it

b30e4b5

adeelibr added 2 commits August 21, 2018 17:25

improvement on active state for thumb

a3c4f77

ran prettier

4fa5ffa

eps1lon requested changes Aug 21, 2018

View reviewed changes

improvements

6b91465

mbrookes requested changes Aug 21, 2018

View reviewed changes

mbrookes and others added 3 commits August 21, 2018 17:15

[docs] Update Slider API docs

fad0478

updated documentation & improvements in thumb

abe9c9c

improved custom icon demo & slider variable names better use

3889145

eps1lon requested changes Aug 22, 2018

View reviewed changes

packages/material-ui-lab/src/Slider/Slider.js Outdated Show resolved Hide resolved

adeelibr added 2 commits August 22, 2018 20:30

active, focus state working properly now

3a858e8

Merge branch 'master' into slider-thumbnail-icon

06ba37f

eps1lon approved these changes Sep 8, 2018

View reviewed changes

mbrookes reviewed Sep 8, 2018

View reviewed changes

adeelibr added 3 commits September 9, 2018 17:08

removed extra space

ba8c1e4

Merge branch 'master' into slider-thumbnail-icon

fea9ed5

Merge branch 'master' into slider-thumbnail-icon

9c6d598

Update API docs

5f29eb3

mbrookes reviewed Sep 11, 2018

View reviewed changes

updated icon

ab55c29

mbrookes requested changes Sep 11, 2018

View reviewed changes

added different id's for different labels

185aa2e

mbrookes reviewed Sep 11, 2018

View reviewed changes

adeelibr added 6 commits September 11, 2018 22:55

removed bloated styling

d19e900

removed withIcon variable

8e503c4

Merge branch 'master' into slider-thumbnail-icon

385b56c

removed redundant git status

7a92eaa

demo revamped

507e0e2

merged master and resolved conflicts

7f7611e

mbrookes approved these changes Sep 13, 2018

View reviewed changes

eps1lon approved these changes Sep 13, 2018

View reviewed changes

mbrookes merged commit 14de82e into mui:master Sep 13, 2018

adeelibr deleted the slider-thumbnail-icon branch September 14, 2018 05:34

marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018

[Slider] Feature Custom Icon (mui#12600)

98da342

oliviertassinari added component: slider This is the name of the generic UI component, not the React module! and removed package: lab Specific to @mui/lab labels Jan 9, 2021

[Slider] Feature Custom Icon #12600

[Slider] Feature Custom Icon #12600

Conversation

adeelibr commented Aug 20, 2018

adeelibr commented Aug 20, 2018

mbrookes commented Aug 20, 2018

adeelibr commented Aug 20, 2018

mbrookes commented Aug 20, 2018

adeelibr commented Aug 20, 2018

eps1lon commented Aug 21, 2018

adeelibr commented Aug 21, 2018

adeelibr commented Aug 21, 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

Choose a reason for hiding this comment

adeelibr commented Aug 21, 2018

mbrookes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adeelibr Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adeelibr Aug 21, 2018 • 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

adeelibr commented Aug 21, 2018

mbrookes commented Aug 22, 2018

eps1lon left a comment • edited Loading

Choose a reason for hiding this comment

adeelibr commented Sep 8, 2018

Choose a reason for hiding this comment

adeelibr commented Sep 9, 2018

adeelibr commented Sep 11, 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

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

mbrookes Sep 11, 2018 • 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

mbrookes commented Sep 13, 2018

eps1lon left a comment • edited Loading

Choose a reason for hiding this comment

mbrookes commented Sep 13, 2018

adeelibr Aug 21, 2018 •

edited

Loading

adeelibr Aug 21, 2018 •

edited

Loading

eps1lon left a comment •

edited

Loading

mbrookes Sep 11, 2018 •

edited

Loading

eps1lon left a comment •

edited

Loading