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

Update theme-manager.js #2527

Merged
merged 3 commits into from
Dec 16, 2015
Merged

Update theme-manager.js #2527

merged 3 commits into from
Dec 16, 2015

Conversation

woshiwenzhijie
Copy link
Contributor

Add function to modify the zIndex of the raw theme

Add function to modify the zIndex of the raw theme
@@ -252,5 +252,13 @@ export default {
let newRawTheme = update(muiTheme.rawTheme, {fontFamily: {$set: newFontFamily}});
return this.getMuiTheme(newRawTheme);
},

//function to modify the zIndex of the raw theme. This function recomputes
Copy link
Member

Choose a reason for hiding this comment

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

you have trailing spaces here

@alitaheri
Copy link
Member

Good feature, thanks a lot 👍 🎉 . Also, please add this to the documentation here as well.

@alitaheri alitaheri added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 15, 2015
@alitaheri alitaheri added this to the 0.14.0 Release milestone Dec 15, 2015
add modifyRawThemeZIndices
@@ -252,5 +252,13 @@ export default {
let newRawTheme = update(muiTheme.rawTheme, {fontFamily: {$set: newFontFamily}});
return this.getMuiTheme(newRawTheme);
},

Copy link
Member

Choose a reason for hiding this comment

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

the space seems to be here. It's failing the build.

@alitaheri
Copy link
Member

Please wait a little before fixing that

@alitaheri
Copy link
Member

I want to hear @oliviertassinari's insight on this. maybe modifyRawThemeZIndex is a more consistent name. Thanks for working on this 👍 and don't for the docs 😁

@oliviertassinari
Copy link
Member

@subjectix I guess it's time that I implement the solution I have proposed here #2460.
This would make the modifyRawThemeZIndices, modifyRawThemeSpacing, modifyRawThemePalette unnecessary.

rename modifyRawThemeZIndices to modifyRawThemeZIndex
@alitaheri
Copy link
Member

@oliviertassinari Hmmmm, you might be right. But these are only helper methods. and we should keep them around for a while. I'd say let's merge this and keep these functions around for a little while before we do a full rewrite and deprecate them all together.

@oliviertassinari
Copy link
Member

I'd say let's merge this and keep these functions around for a little while before we do a full rewrite and deprecate them all together.

Sounds like a plan.

@alitaheri
Copy link
Member

@oliviertassinari Thanks 👍 👍

@woshiwenzhijie Squash down your commits and we'll merge this. 👍 👍

@woshiwenzhijie
Copy link
Contributor Author

😁

@oliviertassinari
Copy link
Member

@woshiwenzhijie As it's your first contribution, I'm gonna merge this PR even those we prefer to have a single commit to have a cleaner history.
Thanks.

oliviertassinari added a commit that referenced this pull request Dec 16, 2015
[Theme] Update theme-manager.js
@oliviertassinari oliviertassinari merged commit 478819c into mui:master Dec 16, 2015
@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 16, 2015
@alitaheri
Copy link
Member

@woshiwenzhijie Thanks 👍 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants