Skip to content
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.

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

support classes prop #25

Merged
merged 3 commits into from
Oct 14, 2017
Merged

support classes prop #25

merged 3 commits into from
Oct 14, 2017

Conversation

jedwards1211
Copy link
Contributor

This allows passing a classes object, and extracts the current classes into the defaultProp value for it. This makes it easy to use with react-jss instead of importing the given styles.css:

import CircularProgress from 'react-circular-progressbar'
import injectSheet from 'react-jss'

const styles = {
  root: {
    width: '100%',
  },
  path: {
    stroke: '#3e98c7',
    strokeLinecap: 'round',
    transition: 'stroke-dashoffset 0.5s ease 0s',
  },
  trail: {
    stroke: '#d6d6d6',
  },
  {
    fill: '#3e98c7';
    fontSize: 20;
    dominantBaseline: 'middle',
    textAnchor: 'middle',
  },
}

export default injectSheet(styles)(CircularProgress)

Copy link
Owner

@kevinsqi kevinsqi left a comment

Choose a reason for hiding this comment

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

cool, thanks for submitting and for the clear explanation.

The classes prop is quite coupled with react-jss. Specifically, it has to be named "classes", or else injectSheet doesn't work (if I'm understanding correctly). I'm not a huge fan of the name "classes", because it seems very easy to confuse with className for users that don't use JSS.

More generally, the usefulness of this prop seems specific to react-jss as well. This prop doesn't help for radium/aphrodite/css-modules, which each require their own inline styling patterns, and it'd be nice for the component to be agnostic about styling libraries.

That being said, there's probably some general utility in adjusting the default classNames even if you don't use react-jss, and my concerns aren't too significant. So this looks good to go - thanks for another PR @jedwards1211!

@kevinsqi kevinsqi merged commit 380baeb into kevinsqi:master Oct 14, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants