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

Improve Overall Theme Handling #2627

Closed
6 of 7 tasks
alitaheri opened this issue Dec 22, 2015 · 12 comments
Closed
6 of 7 tasks

Improve Overall Theme Handling #2627

alitaheri opened this issue Dec 22, 2015 · 12 comments
Labels
umbrella For grouping multiple issues to provide a holistic view

Comments

@alitaheri
Copy link
Member

alitaheri commented Dec 22, 2015

After the huge changes made in #2585 it was decided to break the PR into fractions.

The roadmap for changing themes:

@alitaheri alitaheri added Core umbrella For grouping multiple issues to provide a holistic view labels Dec 22, 2015
@alitaheri alitaheri added this to the 0.14.1 Release milestone Dec 22, 2015
@oliviertassinari
Copy link
Member

Correct me if I'm wrong, but I think that we can add the PureRenderMixin to all of our component as soons as they use the muiTheme HOC 💥

@alitaheri
Copy link
Member Author

@oliviertassinari We should move away from using mixins. Why do you think we should use PureRenderMixin? can't we just use a method to check for changes?

@oliviertassinari
Copy link
Member

@oliviertassinari We should move away from using mixins

Yeah, yeah, that's an implementation detail 😬.
My point is, I think that it's going to be really easy to fix #1176.

@newoga
Copy link
Contributor

newoga commented Dec 22, 2015

I agree we should not use mixins but if @oliviertassinari is right, nothing is stopping us from implementing the pure rendering as another HOC like this. 😄

@alitaheri
Copy link
Member Author

@oliviertassinari I definitely agree with This 👍 👍 Also we won't have to check for context since muiThemeable does it ( I guess this is what you meant 😁 ).

@newoga I totally agree. I'm gonna add this before migrating the components.

Thanks a lot for the feedbacks 👍 👍

@oliviertassinari
Copy link
Member

we won't have to check for context since muiThemeable does it

Well, I have been giving a lot of thought to this issue.
The main point is that we need to have an immutable context variable muiTheme.
The improve theme-manager will merge values, so people will have to use it to change their muiTheme.
That's going to be far easier for people to have an immutable muiTheme context.

Then, I would refer to #1176 (comment).

@alitaheri
Copy link
Member Author

@oliviertassinari Yeah muiTheme should definitely be marked immutable. We better not use any library. We just document the behavior and say: if you want to change the theme you have to make another call to getMuiTheme with whatever property you want to override theme you like before passing it down. And if you want to change theme for a subtree simply wrap the subtree around a MuiThemeProvider. It's rather easy, don't you think?

@oliviertassinari
Copy link
Member

It's rather easy, don't you think?

I completly agree

@alitaheri
Copy link
Member Author

@oliviertassinari Alright. I'm gonna do some research and create a proposal PR with a utility function. 😁 Thanks

@newoga
Copy link
Contributor

newoga commented Dec 22, 2015

@subjectix Didn't get a chance to say thanks for splitting up the work into multiple PRs! So far it's been a lot easier to review. Thanks!

@alitaheri
Copy link
Member Author

@newoga Yeah I was kinda getting a head ache myself 😅 😅

@oliviertassinari oliviertassinari modified the milestones: 0.15.0 Release, 0.16.0 Release Mar 6, 2016
@nathanmarks
Copy link
Member

Closing this in favour of all the other related issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Development

No branches or pull requests

4 participants