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.

Sign up for GitHub

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

Jump to bottom

[Slider] Add support for custom slider icon #12485

Closed

adeelibr wants to merge 3 commits into mui:master from adeelibr:feature-slider-thumbnail

Contributor

adeelibr commented Aug 12, 2018

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

Contributor Author

adeelibr commented Aug 12, 2018

This looks something like this.

adeelibr mentioned this pull request

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

Closed

oliviertassinari added the package: lab label

This comment has been minimized.

Sign in to view

eps1lon requested changes

View reviewed changes

Member

eps1lon left a comment

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

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

@@ @@ -434,6 +444,16 @@ class Slider extends React.Component { @@
                   const inlineTrackAfterStyles = { [trackProperty]: this.calculateTrackAfterStyles(percent) };
                   const inlineThumbStyles = { [thumbProperty]: `${percent}%` };
+                  const Thumbnail = () => {
+                    if (React.isValidElement(Thumb)) {

Member

eps1lon Aug 16, 2018

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.

Contributor Author

adeelibr Aug 16, 2018

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

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

@@ @@ -454,7 +474,9 @@ class Slider extends React.Component { @@
                       <div className={containerClasses}>
                         <div className={trackBeforeClasses} style={inlineTrackBeforeStyles} />
                         <ButtonBase
-                          className={thumbClasses}
+                          className={
+                            Thumbnail() ? classNames(thumbClasses, classes.thumbTransparent) : thumbClasses

Member

eps1lon Aug 16, 2018

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

Contributor Author

adeelibr Aug 16, 2018

I went with option 1 here, updated thumbClasses.

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

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

Member

eps1lon Aug 16, 2018

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

Contributor Author

adeelibr Aug 16, 2018

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

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

@@ @@ -463,7 +485,9 @@ class Slider extends React.Component { @@
                           onTouchStartCapture={this.handleTouchStart}
                           onTouchMove={this.handleMouseMove}
                           onFocusVisible={this.handleFocus}
-                        />
+                        >
+                          {Thumbnail()}

Member

eps1lon Aug 16, 2018

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.

Contributor Author

adeelibr Aug 16, 2018

Fixed.

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

@@ @@ -524,6 +548,7 @@ Slider.propTypes = { @@
                  * @ignore
                  */
                 theme: PropTypes.object.isRequired,
+                thumb: PropTypes.oneOfType([PropTypes.string, PropTypes.func, PropTypes.object]),

Member

eps1lon Aug 16, 2018

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

Contributor Author

adeelibr Aug 16, 2018

Fixed.

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

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

Member

eps1lon Aug 16, 2018

Please add a block comment to document this className.

Contributor Author

adeelibr Aug 16, 2018

I have added a small documentation to it.

Contributor Author

adeelibr commented Aug 16, 2018

@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.

Contributor Author

adeelibr commented Aug 16, 2018

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..

Member

eps1lon commented Aug 16, 2018

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.

Contributor Author

adeelibr commented Aug 17, 2018 •

edited

Loading

@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?

Member

eps1lon commented Aug 18, 2018

@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.

Contributor Author

adeelibr commented Aug 18, 2018

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

Member

eps1lon commented Aug 18, 2018

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

Contributor Author

adeelibr commented Aug 18, 2018

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

Member

eps1lon commented Aug 18, 2018

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.

Contributor Author

adeelibr commented Aug 18, 2018

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

Member

eps1lon commented Aug 19, 2018

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.

Contributor Author

adeelibr commented Aug 19, 2018 •

edited

Loading

@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

Member

eps1lon commented Aug 19, 2018

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.

Contributor Author

adeelibr commented Aug 19, 2018

@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.

Contributor Author

adeelibr commented Aug 19, 2018

still failed..

Member

eps1lon commented Aug 19, 2018 •

edited

Loading

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.

Contributor Author

adeelibr commented Aug 19, 2018

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..

Member

eps1lon commented Aug 19, 2018

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.

Contributor Author

adeelibr commented Aug 19, 2018

@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.

Contributor Author

adeelibr commented Aug 19, 2018

@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.

Member

oliviertassinari commented Aug 19, 2018

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

Contributor Author

adeelibr commented Aug 19, 2018

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 mentioned this pull request

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

Merged

Contributor Author

adeelibr commented Aug 20, 2018 •

edited

Loading

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

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

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

adeelibr deleted the feature-slider-thumbnail branch

August 20, 2018 21:58

adeelibr mentioned this pull request

[Slider] Feature Custom Icon #12600

Merged

oliviertassinari added component: slider new feature and removed package: lab labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: slider new feature