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.

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

[Progress] Add color property #7500

Merged
merged 9 commits into from
Jul 22, 2017
Merged

[Progress] Add color property #7500

merged 9 commits into from
Jul 22, 2017

Conversation

kgregory
Copy link
Member

  • Addresses issue [Progress] Support the color prop #7221
  • Add color property to CircularProgress and LinearProgress, accepts accent and primary (default)
  • Add examples to component demo in docs
  • Adds tests
  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@kgregory
[Progress] Support 'color' property
11b6b92 
- Add color property to CircularProgress and LinearProgress
- Add examples in docs
@kgregory
Merge branch 'v1-alpha' of https://github.com/callemall/material-ui i…
894fa28 
…nto v1-alpha
@kgregory
[Progress] New color property
e8f006f 
- Eliminate errant use of  'default' color in LinearProgress
- Unit tests for CircularProgress
- Unit tests for LinearProgress
@kgregory
[Progress] Add new color property
7b0ac7f 
- API docs
@oliviertassinari
Copy link
Member

Could you remove the package.lock? I mean, why should we keep it?

@kgregory
[Progress] Add color property
71e02a5 
- did not mean to include package-lock.json
@kgregory
Copy link
Member Author

@oliviertassinari npm 5 got me - it's gone. Sorry! I didn't mean to include it.

oliviertassinari
oliviertassinari reviewed Jul 21, 2017
View reviewed changes
src/Progress/LinearProgress.js Outdated
@@ -8,38 +8,56 @@ import withStyles from '../styles/withStyles';

const transitionDuration = 4; // 400ms

const getBar = color => ({
Copy link
Member

Choose a reason for hiding this comment

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

Should like we have duplicated style injected in the page. What about sharing what's common between the colors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just updated now. The only redundancy is that the background attributes for the color-specific dashed classes.

@Airblader
Copy link
Contributor

@oliviertassinari

Could you remove the package.lock? I mean, why should we keep it?

It is meant to be committed, but if you want to handle it otherwise, we should probably add it to .gitignore.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2017 •
edited
Loading

I have found that thread yarnpkg/yarn#3614. Let's follow 1.

@Airblader I agree adding it in the .gitignore sounds like the best plan. What do you think?

Airblader added a commit to Airblader/material-ui that referenced this pull request Jul 21, 2017
@Airblader
Ignore the package-lock.json file generated by npm.
86b66ef 
See PR mui#7500 for a discussion.
oliviertassinari pushed a commit that referenced this pull request Jul 21, 2017
@Airblader @oliviertassinari
Ignore the package-lock.json file generated by npm. (#7502)
bb1137e 
See PR #7500 for a discussion.
@kgregory
Merge branch 'v1-alpha' of https://github.com/callemall/material-ui i…
d803def 
…nto v1-alpha
@kgregory
[Progress] Add color property
d3c9fad 
- Share as many of the properties as possible
- bar, dashed, bufferBar2 baseclasses added back, functions eliminated
oliviertassinari
oliviertassinari approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

One last comment, that looks great 👍

src/Progress/LinearProgress.js Outdated
backgroundColor: theme.palette.accent.A100,
},
accentColorBar: {
backgroundColor: theme.palette.accent.A200,
Copy link
Member

Choose a reason for hiding this comment

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

What about A400? I don't think that we have enough contrast otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with this. I'll update it when I get a chance tomorrow.

@oliviertassinari oliviertassinari merged commit dcf921c into mui:v1-alpha Jul 22, 2017
@oliviertassinari
Copy link
Member

@kgregory Updated :)

@kgregory
Copy link
Member Author

Thanks @oliviertassinari!

@oliviertassinari oliviertassinari changed the title [Progress] Add color property (issue #7221) [Progress] Add color property Jul 23, 2017
@kgregory kgregory deleted the v1-alpha branch July 25, 2017 13:44
@oliviertassinari oliviertassinari added the component: progress This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
component: CircularProgress The React component component: LinearProgress The React component. component: progress This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kgregory @oliviertassinari @Airblader