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

[Card] Remove style-propable mixin #3198

Merged
merged 1 commit into from
Feb 8, 2016
Merged

[Card] Remove style-propable mixin #3198

merged 1 commit into from
Feb 8, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Feb 6, 2016

No description provided.

@yongxu yongxu mentioned this pull request Feb 6, 2016
Closed
90 tasks
@newoga newoga added the Core label Feb 6, 2016
@newoga
Copy link
Contributor Author

newoga commented Feb 6, 2016

I noticed a few of these components imported from ./styles so that it could use typography and colors. I switched them to just use typography because everything they needed was in there, but at some point maybe we should just make those colors themeable and have them pull from muiTheme rather than from import. Maybe as a general rule it would be good to have components not import from the './styles' folder and make everything in theme.

@alitaheri
Copy link
Member

@newoga That should be how it is. importing from styles means it won't be customizable. So that shouldn't be how it works. 👍

#2892 is addressing some of them 😄

@newoga
Copy link
Contributor Author

newoga commented Feb 6, 2016

@alitaheri Oh right, I totally forgot about that PR! 👍

It seems like that PR addresses the very components I'm talking about too! Awesome, I probably should've waited until that was merged before starting these components 😅

@alitaheri
Copy link
Member

I was worried about conflicts too, but then I looked at both sources and I don't see conflicts. even if there are. they will be vary minor. so no worries 😁

@newoga
Copy link
Contributor Author

newoga commented Feb 6, 2016

Okay awesome! 😄

oliviertassinari
oliviertassinari reviewed Feb 6, 2016
View reviewed changes
src/card/card-text.jsx
return {
root: {
padding: 16,
fontSize: '14px',
Copy link
Member

Choose a reason for hiding this comment

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

fontSize: 14,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@newoga
Copy link
Contributor Author

newoga commented Feb 6, 2016

Fixed 14px fontSize to 14 and removed unnecessary {} in Object.assign

oliviertassinari
oliviertassinari reviewed Feb 6, 2016
View reviewed changes
src/card/card.jsx
@@ -107,7 +102,7 @@ const Card = React.createClass({
...other,
} = this.props;

let mergedStyles = this.mergeStyles({
const mergedStyles = Object.assign({}, {
Copy link
Member

Choose a reason for hiding this comment

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

Object.assign({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@newoga
Copy link
Contributor Author

newoga commented Feb 6, 2016

Fixed two unnecessary {} in Object.assign

@newoga
Copy link
Contributor Author

newoga commented Feb 7, 2016

Just rebased onto master to get changes from #3215.

@oliviertassinari
Copy link
Member

@newoga The travis task fails 😁.

@newoga
Copy link
Contributor Author

newoga commented Feb 7, 2016

@oliviertassinari Looks like a missing space. I'll fix it. Thanks!

@newoga
Copy link
Contributor Author

newoga commented Feb 7, 2016

Fixed!

oliviertassinari
oliviertassinari reviewed Feb 8, 2016
View reviewed changes
src/card/card-expandable.jsx
getStyles() {
const contextKeys = this.constructor.getRelevantContextKeys(this.state.muiTheme);

const directionStyle = contextKeys.isRtl ? {
Copy link
Member

Choose a reason for hiding this comment

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

I think that the isRtl logic is missing.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, rtl(muiTheme) should handle it correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tested this and realized it was a bug because it was reversing the rtl logic 😁

@oliviertassinari
Copy link
Member

@newoga Thanks!

oliviertassinari added a commit that referenced this pull request Feb 8, 2016
@oliviertassinari
Merge pull request #3198 from newoga/#2852/card
d88e943 
[Card] Remove style-propable mixin
@oliviertassinari oliviertassinari merged commit d88e943 into mui:master Feb 8, 2016
@newoga newoga deleted the #2852/card branch February 8, 2016 16:26
@zannager zannager added the component: card This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: card 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.
4 participants
@newoga @alitaheri @oliviertassinari @zannager