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

[Core] Typography moved inside muitheme #3301

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

heetvachhani
Copy link
Contributor

Reference to #2908

@oliviertassinari oliviertassinari changed the title [Docs] Typography moved inside muitheme [Core] Typography moved inside muitheme Feb 11, 2016
@@ -255,14 +254,16 @@ const FlatButton = React.createClass({
const buttonRippleColor = rippleColor || defaultRippleColor;
const buttonBackgroundColor = backgroundColor || buttonColor;
const hovered = (this.state.hovered || this.state.isKeyboardFocused) && !disabled;
const fontSize = this.state.muiTheme.flatButton.fontSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, this can also be written like this:

const {
  fontSize,
  fontWeight,
} = this.state.muiTheme.flatButton;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I update this change @newoga?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be okay for now, what do you think @oliviertassinari @alitaheri?

If the the theme changes are solid, I'm good merging this as is. I think for this file we'll have to revisit to removing ContextPure and getRelevantContextKeys. That might be a good opportunity to reorganize the assignment of theme variables.

Copy link
Member

Choose a reason for hiding this comment

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

We'll have to revisit all the files again. so this is alright in my book 😁

@newoga
Copy link
Contributor

newoga commented Feb 12, 2016

Thanks @heetvachhani, good work!

@heetvachhani
Copy link
Contributor Author

okay not a problem. You're welcome @newoga 😄

@alitaheri
Copy link
Member

This is really lovely 😍 Thanks a lot @heetvachhani

All green on ly end 👍

oliviertassinari added a commit that referenced this pull request Feb 12, 2016
[Core] Typography moved inside muitheme
@oliviertassinari oliviertassinari merged commit 0e785fa into mui:master Feb 12, 2016
@oliviertassinari
Copy link
Member

@heetvachhani Thanks! Can we deprecate the Typography file now and close #2908?

@heetvachhani
Copy link
Contributor Author

@oliviertassinari so what I have to do for that?
You're welcome @alitaheri @oliviertassinari

@oliviertassinari
Copy link
Member

Can we deprecate the Typography file now

Actually, that may not be a good idea. That make sense to keep it, as for the colors.

@newoga
Copy link
Contributor

newoga commented Feb 12, 2016

Actually, that's may not be a good idea. That make sense to keep it as for the colors.

Yeah I was wondering the same. Let's hold off, sometimes I wonder if material-ui could still use some tooling to help developers implement some of the ideas discussed here in the spec.

@heetvachhani
Do you want to do the same thing you did in this PR but do it for ./Colors. A number of components directly import directly from colors and color manipulator, there might be opportunities to use theme variables for that as well.

@heetvachhani
Copy link
Contributor Author

yes sure I can do that. @newoga. Just give me the list of components and will do it.

@alitaheri
Copy link
Member

@heetvachhani You can get the list and the exact position from my PR:
#2825

I can't merge that because it breaks the docs. but if you remove all the usage of colors from these files I can merge that PR too. 2 birds with 1 stone 😍 😍

@heetvachhani
Copy link
Contributor Author

Thanks @alitaheri it will make my work easy . cool 2 birds with 1 "arrow" 😄

@heetvachhani heetvachhani deleted the Move_Typography branch February 15, 2016 20:57
@zannager zannager added component: Typography The React component. core Infrastructure work going on behind the scenes labels Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants