Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
979c0cd
feature slider icon
adeelibr Aug 20, 2018
cac07cd
[docs] Update Slider API docs
mbrookes Aug 20, 2018
49b716f
updated demo
adeelibr Aug 20, 2018
201177e
added custom description
adeelibr Aug 20, 2018
1efcbf3
corrected heading
adeelibr Aug 20, 2018
5933838
ran prettier to format code
adeelibr Aug 20, 2018
b30e4b5
improvements on thumb icon while draggin it
adeelibr Aug 21, 2018
a3c4f77
improvement on active state for thumb
adeelibr Aug 21, 2018
4fa5ffa
ran prettier
adeelibr Aug 21, 2018
6b91465
improvements
adeelibr Aug 21, 2018
fad0478
[docs] Update Slider API docs
mbrookes Aug 21, 2018
abe9c9c
updated documentation & improvements in thumb
adeelibr Aug 21, 2018
3889145
improved custom icon demo & slider variable names better use
adeelibr Aug 21, 2018
3a858e8
active, focus state working properly now
adeelibr Aug 22, 2018
06ba37f
Merge branch 'master' into slider-thumbnail-icon
adeelibr Aug 23, 2018
c4639d0
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 2, 2018
46750f3
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 4, 2018
cd6f752
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 6, 2018
617bf64
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 8, 2018
ba8c1e4
removed extra space
adeelibr Sep 9, 2018
fea9ed5
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 9, 2018
9c6d598
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 9, 2018
5f29eb3
Update API docs
mbrookes Sep 9, 2018
ab55c29
updated icon
adeelibr Sep 11, 2018
185aa2e
added different id's for different labels
adeelibr Sep 11, 2018
d19e900
removed bloated styling
adeelibr Sep 11, 2018
8e503c4
removed withIcon variable
adeelibr Sep 11, 2018
385b56c
Merge branch 'master' into slider-thumbnail-icon
adeelibr Sep 11, 2018
7a92eaa
removed redundant git status
adeelibr Sep 11, 2018
507e0e2
demo revamped
adeelibr Sep 12, 2018
7f7611e
merged master and resolved conflicts
adeelibr Sep 12, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions docs/src/pages/lab/slider/CustomIconSlider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
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 StarsIcon from '@material-ui/icons/Gamepad';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GamepadIcon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Updating it :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const styles = {
root: {
width: 300,
},
thumbIcon: {
borderRadius: '50%',
},
};

class CustomIconSlider 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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Image thumb"

<Slider
value={value}
aria-labelledby="label"
onChange={this.handleChange}
thumb={
<img
alt="slider thumb icon"
src="/static/images/cards/live-from-space.jpg"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove this image example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this image, then yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, understood.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

className={classes.thumbIcon}
/>
}
/>
<Typography id="label">Slider Icon</Typography>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Icon thumb"

<Slider
value={value}
aria-labelledby="label"
onChange={this.handleChange}
thumb={<StarsIcon style={{ color: '#2196f3' }} />}
/>
</div>
);
}
}

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

export default withStyles(styles)(CustomIconSlider);
4 changes: 4 additions & 0 deletions docs/src/pages/lab/slider/slider.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,7 @@ Sliders reflect the current state of the settings they control.
## Reverse slider

{{"demo": "pages/lab/slider/ReverseSlider.js"}}

## Custom thumb

{{"demo": "pages/lab/slider/CustomIconSlider.js"}}
41 changes: 38 additions & 3 deletions packages/material-ui-lab/src/Slider/Slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,17 @@ export const styles = theme => {
height: 17,
},
},
/* Class applied to the thumb element if custom thumb icon provided` . */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

thumbIconWrapper: {
backgroundColor: 'transparent',
},
thumbIcon: {
position: 'relative',
top: 0,
height: 26,
width: 'auto',
zIndex: 2,
},
/* Class applied to the root element to trigger JSS nested styles if `reverse={true}` . */
reverse: {},
/* Class applied to the track and thumb elements to trigger JSS nested styles if `disabled`. */
Expand Down Expand Up @@ -375,6 +386,7 @@ class Slider extends React.Component {
const { currentState } = this.state;
const {
component: Component,
thumb: Thumb,
classes,
className: classNameProp,
disabled,
Expand Down Expand Up @@ -418,14 +430,30 @@ class Slider extends React.Component {
[classes.vertical]: vertical,
});

const thumbClasses = classNames(classes.thumb, commonClasses);

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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@adeelibr adeelibr Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thumbIcon?

Copy link
Contributor Author

@adeelibr adeelibr Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const withIcon = !!Thumb;
const Thumbnail = withIcon
? React.cloneElement(Thumb, {
...Thumb.props,
className: `${classes.thumbIcon} ${Thumb.props.className || ''}`,
})
: null;
/** End Thumbnail Icon Logic Here */

const thumbClasses = classNames(
{
[classes.thumb]: !withIcon,
[classes.thumbIconWrapper]: withIcon,
},
commonClasses,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, i'll do something about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return (
<Component
role="slider"
Expand Down Expand Up @@ -454,7 +482,9 @@ class Slider extends React.Component {
onTouchStartCapture={this.handleTouchStart}
onTouchMove={this.handleMouseMove}
onFocusVisible={this.handleFocus}
/>
>
{Thumbnail}
</ButtonBase>
<div className={trackAfterClasses} style={inlineTrackAfterStyles} />
</div>
</Component>
Expand Down Expand Up @@ -515,6 +545,11 @@ Slider.propTypes = {
* @ignore
*/
theme: PropTypes.object.isRequired,
/**
* The component used for the slider icon.
* This is optional, if provided should be a react element.
*/
thumb: PropTypes.element,
/**
* The value of the slider.
*/
Expand Down
3 changes: 3 additions & 0 deletions pages/lab/api/slider.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ title: Slider API
| <span class="prop-name">onDragStart</span> | <span class="prop-type">func |   | Callback function that is fired when the slider has begun to move. |
| <span class="prop-name">reverse</span> | <span class="prop-type">bool |   | If `true`, the slider will be reversed. |
| <span class="prop-name">step</span> | <span class="prop-type">number |   | The granularity the slider can step through values. |
| <span class="prop-name">thumb</span> | <span class="prop-type">element |   | The component used for the slider icon. This is optional, if provided should be a react element. |
| <span class="prop-name required">value *</span> | <span class="prop-type">number |   | The value of the slider. |
| <span class="prop-name">vertical</span> | <span class="prop-type">bool |   | If `true`, the slider will be vertical. |

Expand All @@ -44,6 +45,8 @@ This property accepts the following keys:
| <span class="prop-name">trackBefore</span> | Styles applied to the track element before the thumb.
| <span class="prop-name">trackAfter</span> | Styles applied to the track element after the thumb.
| <span class="prop-name">thumb</span> | Styles applied to the thumb element.
| <span class="prop-name">thumbTransparent</span> | Class applied to the thumb element if custom thumb icon provided` .
| <span class="prop-name">thumbIcon</span> |
| <span class="prop-name">reverse</span> | Class applied to the root element to trigger JSS nested styles if `reverse={true}` .
| <span class="prop-name">disabled</span> | Class applied to the track and thumb elements to trigger JSS nested styles if `disabled`.
| <span class="prop-name">jumped</span> | Class applied to the track and thumb elements to trigger JSS nested styles if `jumped`.
Expand Down
7 changes: 7 additions & 0 deletions pages/lab/slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ module.exports = require('fs')
raw: preval`
module.exports = require('fs')
.readFileSync(require.resolve('docs/src/pages/lab/slider/ReverseSlider'), 'utf8')
`,
},
'pages/lab/slider/CustomIconSlider.js': {
js: require('docs/src/pages/lab/slider/CustomIconSlider').default,
raw: preval`
module.exports = require('fs')
.readFileSync(require.resolve('docs/src/pages/lab/slider/CustomIconSlider'), 'utf8')
`,
},
}}
Expand Down