[slider] Add support for custom slider icon by adeelibr · Pull Request #12485 · mui/material-ui

adeelibr · 2018-08-12T15:11:26Z

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);

adeelibr · 2018-08-12T15:12:07Z

This looks something like this.

eps1lon

When you have added the requested block comments run yarn docs:api.

Please add tests and examples to the documentation (see pages/lab/slider.js).

eps1lon · 2018-08-16T04:39:06Z

    const inlineThumbStyles = { [thumbProperty]: `${percent}%` };

+    const Thumbnail = () => {
+      if (React.isValidElement(Thumb)) {


I'm not sure what the practice is in this package but if you would require the React.element prop-type this check would be redundant.

Changed prop type to React.element, and removed this extra flag.

eps1lon · 2018-08-16T04:47:35Z

          <ButtonBase
-            className={thumbClasses}
+            className={
+              Thumbnail() ? classNames(thumbClasses, classes.thumbTransparent) : thumbClasses


Two possibilities:

Wouldn't it be more "semantic" to add a with-icon class (something along the line) in thumbClasses and let classNames to the conditional logic stuff?

Use IconButton if a thumb elment is provided instead of ButtonBase (which I'd prefer).

I went with option 1 here, updated thumbClasses.

eps1lon · 2018-08-16T04:55:32Z

+      if (React.isValidElement(Thumb)) {
+        return React.cloneElement(Thumb, {
+          ...Thumb.props,
+          className: classes.thumbIcon,


This overwrites existing classNames on the thumb. It should respect existing classNames.

I have made changes now if the user adds classes to the thumb, it won't get over written.

eps1lon · 2018-08-16T05:00:33Z

            onFocusVisible={this.handleFocus}
-          />
+          >
+            {Thumbnail()}


I would change the Thumbnail assignment so that you don't have to use this call syntax here. It's more in line with other components and also removes a second call to the function.

eps1lon · 2018-08-16T05:02:28Z

   * @ignore
   */
  theme: PropTypes.object.isRequired,
+  thumb: PropTypes.oneOfType([PropTypes.string, PropTypes.func, PropTypes.object]),


Please add a block comment and document this property and use PropTypes.element.

eps1lon · 2018-08-16T05:03:00Z

+    thumbTransparent: {
+      backgroundColor: 'transparent',
+    },
+    thumbIcon: {


Please add a block comment to document this className.

I have added a small documentation to it.

adeelibr · 2018-08-16T18:22:27Z

@eps1lon first of all thank you so much for reviewing my PR, I have made changes as per your instructions. Do let me know if it still needs any tweaking.

adeelibr · 2018-08-16T18:31:50Z

I think I made a mistake, I took a pull master and merged it in my feature-slider-thumbnail branch. And now there is a lot of code in the PR.. what should I do?

Also I have no idea that why in slider.test.js this test case is failing `should call handlers' I did nothing with that part of code..

eps1lon · 2018-08-16T20:32:25Z

The failing tests should've been resolve with #12535. Try rebasing or merging master again.

Runningyarn docs:api should limit the changed files to your feature only.

For general advice about PR workflow I recommend https://gist.github.com/Chaser324/ce0505fbed06b947d962.

adeelibr · 2018-08-17T18:09:38Z

@eps1lon I took a pull and merged this branch with master again, on running the tests after that merge all the tests where passing. But the Circleci build is failing.

A bit information on how I ran the test, for Slider
I went in

cd {ROOT_FOLDER_MATERIAL_UI}/packages/material-ui-lab
yarn test

It ran the test and gave an output for 78 passing (744ms)

After that when the build failed, I in my root folder ran yarn test It ran for a while and gave an output for 1609 passing (5s) but why is the CircleCI build still failing?

eps1lon · 2018-08-18T09:33:05Z

@adeelibr CircleCI is just broken at the moment (https://discuss.circleci.com/t/saving-cache-stopped-working-warning-skipping-this-step-disabled-in-configuration/24423/2). Nothing you can do about it but patiently wait for a fix.

adeelibr · 2018-08-18T10:29:05Z

@eps1lon so does that mean my PR is okay, that it can be merged?

eps1lon · 2018-08-18T11:03:07Z

Rebase/merge again to include #12573 and we'll see if everything is ok.

adeelibr · 2018-08-18T14:06:23Z

One test is still failing ci/circleci: test_browser after the merge including #12573

eps1lon · 2018-08-18T15:05:23Z

The branch is just very messy due to all the merge commits. In general if any Should not have any git stage fails you should check what command was executed by the CI pipeline before the failed command, run that command locally and commit the changes generated by that task.

In this case yarn docs:api.

adeelibr · 2018-08-18T16:01:25Z

I have ran yarn docs:api updated the master branch and merged it in this feature branch, still failing.

eps1lon · 2018-08-19T13:27:36Z

Looks like you ran docs:api before the merge. When I checkout this PR and run docs:api I get the expected changes that should be committed.

adeelibr · 2018-08-19T14:28:59Z

@eps1lon i don't know what's happening this is what I am doing right now

git checkout master
git pull upstream master
git checkout feature-branch-name
git merge master
git push origin feature-branch-name

eps1lon · 2018-08-19T14:30:56Z

You are not running yarn docs:api after the merge. This should change many files. All of those changes need to be committed and pushed.

adeelibr · 2018-08-19T14:39:53Z

@eps1lon sorry, this is what i was doing

git checkout master
git pull upstream master
git checkout feature-branch-name
git merge master
yarn docs:api
git commit -am "some message"
git push origin feature-branch-name

what i did was I updated, one of my props definition that I added in this PR ran yarn docs:api again, committed the code and pushed it. I hope this works now.

adeelibr · 2018-08-19T14:40:31Z

still failed..

eps1lon · 2018-08-19T14:55:12Z

I'm pretty sure you have your line endings set to \r\n instead of \n. Check out https://help.github.com/articles/dealing-with-line-endings/ and change your setup so that \n is saved as the line ending only.

This should probably be mentioned in Contributing.md. Asking @oliviertassinari if we should document the behavior, warn if we detect \r\n when running yarn docs:api or change the regex https://github.com/mui-org/material-ui/blob/823fed03727bb6b52dbfc329472640b8e9da0e21/docs/scripts/buildApi.js#L112-L113

Perfect solution would be to run generateMarkdown on an AST instead of raw source to avoid future formatting issues.

adeelibr · 2018-08-19T15:10:17Z

I did this

git config --global core.autocrlf true

And ran yarn docs:api but no changes to push.

I then saw the .gitattributes file
https://github.com/mui-org/material-ui/blob/823fed03727bb6b52dbfc329472640b8e9da0e21/.gitattributes#L2

It is already configured for the use case that you have mentioned above, so back at square one I think..

eps1lon · 2018-08-19T15:19:12Z

It should be set to text eol=lf for that reason. Set your global to git config --global core.autocrlf input and then change your IDE to use LF as line-ending.

adeelibr · 2018-08-19T16:00:16Z

@eps1lon I have changed it from CRLF to LF, changed my git configuration's git config --global core.autocrlf input added LF support in my IDE which is VSCODE "files.eol": "\n", now when I run `yarn docs:api" it does remove the previous files and when it generates new files they are still CRLF by default.

adeelibr · 2018-08-19T16:10:50Z

@oliviertassinari can we somehow change the regex https://github.com/mui-org/material-ui/blob/823fed03727bb6b52dbfc329472640b8e9da0e21/docs/scripts/buildApi.js#L112-L113 so that it is not dependent on the user side.

oliviertassinari · 2018-08-19T19:02:50Z

@adeelibr I would try to replace the \n with [\r\n].

adeelibr · 2018-08-19T19:39:15Z

I have made another PR for this, to resolve this CRLF to LF issue. #12584 I have updated the regex, but it's still giving me the same error. @oliviertassinari

adeelibr · 2018-08-20T19:19:39Z

This is what I did

git checkout master
git pull upstream master
git checkout feature-branch-name
git merge master

Then I made some changes in only the files I wanted to be committed. Which were;
1- docs/src/pages/lab/slider/CustomIconSlider.js
2- docs/src/pages/lab/slider/slider.md
3- packages/material-ui-lab/src/Slider/Slider.js
4- pages/lab/slider.js

After that

git commit -am "some message" // this only added those 4 files I modified nothing else.
git push --force origin feature-branch-name

Isn't this how you wanted me to do it. @oliviertassinari

But it committed those docs files as well. I am really bad at this.. sigh

…o use classNames, class overirde issue resolved, changes Thumbnail assignment instead of call, added comments

adeelibr mentioned this pull request Aug 12, 2018

[slider] Make thumb in slider component more customizable. #12438

Closed

oliviertassinari added the package: lab Specific to the lab. label Aug 14, 2018

This comment has been minimized.

Sign in to view

eps1lon requested changes Aug 16, 2018

View reviewed changes

adeelibr mentioned this pull request Aug 20, 2018

[core] Better Windows support for the API generation #12584

Merged

adeelibr changed the title ~~Feature slider thumbnail~~ [Slider] Add support for custom slider icon Aug 20, 2018

adeelibr added 2 commits August 21, 2018 00:58

initial commit

61595b5

Added Documentation, Added prop validation, improved the way on how t…

432ed0b

…o use classNames, class overirde issue resolved, changes Thumbnail assignment instead of call, added comments

adeelibr force-pushed the feature-slider-thumbnail branch 2 times, most recently from a07aa25 to 432ed0b Compare August 20, 2018 21:02

git reset to working docs

7fe6bd4

adeelibr closed this Aug 20, 2018

adeelibr deleted the feature-slider-thumbnail branch August 20, 2018 21:58

adeelibr mentioned this pull request Aug 20, 2018

[slider] Feature Custom Icon #12600

Merged

oliviertassinari added scope: slider Changes related to the slider. type: new feature Expand the scope of the product to solve a new problem. and removed package: lab Specific to the lab. labels Jan 9, 2021

oliviertassinari changed the title ~~[Slider] Add support for custom slider icon~~ [slider] Add support for custom slider icon May 9, 2026

Uh oh!

Uh oh!

Conversation

adeelibr commented Aug 12, 2018

Uh oh!

adeelibr commented Aug 12, 2018

Uh oh!

This comment has been minimized.

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adeelibr commented Aug 16, 2018

Uh oh!

adeelibr commented Aug 16, 2018

Uh oh!

eps1lon commented Aug 16, 2018

Uh oh!

adeelibr commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon commented Aug 18, 2018

Uh oh!

adeelibr commented Aug 18, 2018

Uh oh!

eps1lon commented Aug 18, 2018

Uh oh!

adeelibr commented Aug 18, 2018

Uh oh!

eps1lon commented Aug 18, 2018

Uh oh!

adeelibr commented Aug 18, 2018

Uh oh!

eps1lon commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 19, 2018

Uh oh!

eps1lon commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adeelibr commented Aug 19, 2018

Uh oh!

eps1lon commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 19, 2018

Uh oh!

oliviertassinari commented Aug 19, 2018

Uh oh!

adeelibr commented Aug 17, 2018 •

edited

Loading

adeelibr commented Aug 19, 2018 •

edited

Loading

eps1lon commented Aug 19, 2018 •

edited

Loading

adeelibr commented Aug 20, 2018 •

edited

Loading