Proposal: mui-themeable higher order component #2493

newoga · 2015-12-12T17:44:10Z

I'm just opening this up for discussion, something we could consider at a later date.

The React documentation on context says:

Regardless of whether you're building an application or a library, try to isolate your use of context to a small area and avoid using the context API directly when possible so that it's easier to upgrade when the API changes.

This pull request shows an approach to implementing context for the purpose of themes in a higher order mui-themeable.js component. It allows us to develop material-ui components without having to use context directly, and instead receive the muiTheme as a prop (even though the higher order component is providing the muiTheme prop from context). I also changed the current Divider.jsx implementation as an example of how it's used.

Note: This current implementation technically should work as a @muiThemeable decorator on classes as well, but since Divider.jsx is a function, I couldn't show that as an example.

alitaheri · 2015-12-12T20:31:13Z

About time someone addressed this :D Thanks a lot. @oliviertassinari I think this should be merged asap so while we work on the docs we adept the components too.

alitaheri · 2015-12-12T20:35:08Z

@newoga Could you be so kind and provide a ThemeProvider component so that people will add that to that top level component to pass down the theme, This component will simply take a theme object and pass it down the tree as context for those wrapped with muiThemeable.

Oh and document it too :D sorry 😅

newoga · 2015-12-12T23:31:42Z

@subjectix Sure, that makes sense. I should have time later tonight or tomorrow to whip up a first iteration of a ThemeProvider. Let me know if you think there we should name any of these files/functions differently as I haven't given it too much thought.

And no apologies on documentation, I like documentation and I'm here to help 😄

newoga · 2015-12-12T23:40:17Z

Note: Just wanted to write this so I don't forget when I come back to this. In this pull request's implementation, the muiTheme is only provided through context. If we wanted the ability to allow users to override the theme by providing their own theme as a prop to a component, we'll have to add a check for that.

Maybe something like:

/* excerpt from mui-themeable.js */
const MuiComponent = (props, {contextTheme = ThemeManager.getMuiTheme(DefaultRawTheme)}) =>
{
  const {
    muiTheme = contextTheme
  } = props;

  return <WrappedComponent {...props} muiTheme={muiTheme} />;
};

Maybe there's a nicer way to write that.

alitaheri · 2015-12-13T11:48:24Z

to allow users to override the theme by providing their own theme as a prop

I would argue that it's more explicit to wrap that component they want to customize around the same ThemeProvider component as they would with the top level component. don't you think it makes more sense to end users this way? It will be like "this subtree should use this theme" :D

Besides if the theme is not provided through context, but rather through a prop, the how will the Children of that component look like? they won't know the theme was changed for their parent. they will still try to get it from the context (which is inherited from the ancestor).

newoga · 2015-12-13T18:12:27Z

@subjectix That's a good point, I hadn't considered that and I think you're right on all accounts. I'm taking a stab at the ThemeProvider now.

alitaheri · 2015-12-13T18:33:04Z

Thanks @newoga, I really like your approach 👍 This is the right way to do this 👍 😄

newoga · 2015-12-13T19:21:40Z

Thanks @subjectix! This should do it. Quick question, I wrote the mui-theme-provider.jsx component as an ES6 class. Is that okay? Or is the plan to write everything as ES5, and do a complete conversion to ES6 in the es6-branch?

I confirmed that this is more or less working locally, but I'm going to try to create some official examples in docs and push again. I might have some other ideas and concerns (nothing show stopping) but I'll experiment a bit more and report back soon.

neverfox · 2015-12-13T19:25:47Z

@newoga Not to be sour (because I love this), but should this perhaps be implemented without using a stateless functional component? There are still some issues with these when it comes to hot reloading (more here).

newoga · 2015-12-13T19:40:30Z

@neverfox, thanks for the feedback and pointing that out! I tend to write components stateless and functional by default until there's a reason to not to 😅 . That could very well be a good enough reason not to and am open to switching it to a class implementation.

It seems like stateless functional components are being pushed and encouraged across the board, but tooling for it hasn't fully caught up for it yet. Let me know if you have any other thoughts.

neverfox · 2015-12-13T19:43:02Z

@newoga I do too, since I build all of my apps with redux. My only thought about why this isn't going to be a problem is that this isn't code that a developer would necessarily be changing in development (so nothing to hot reload). If my thinking is correct on that, then my concern is probably misplaced. Unfortunately, it is a reason why I don't currently use the pattern for my own app components yet.

newoga · 2015-12-13T19:44:53Z

Out of curiosity @neverfox, do you know if the implications of using a stateless functional component in this particular case only relevant for hot reloading when changing material-ui source code? Or does it impact editing any source code in user space that includes material-ui components?

Let me know if that question makes sense.

newoga · 2015-12-13T19:45:07Z

Haha! We both thought of it at the same time.

neverfox · 2015-12-13T19:46:57Z

@newoga I don't know for sure, but I would expect that it wouldn't be an issue if you weren't modifying material-ui source on-the-fly because the stateless material-ui component would re-render normally when and if props change, but that is something to explore.

newoga · 2015-12-13T19:49:05Z

Right, I was thinking the same. But you're right, this is something to explore. Before we merge this, I'll try to experiment to see what impact this could have in that area.

I know babel-plugin-react-transform is going through a lot of refactoring and fixes. The good news is it seems like the greater community is moving towards supporting these patterns rather than away.

alitaheri · 2015-12-13T19:52:37Z

and do a complete conversion to ES6 in the es6-branch

This is like an ahead-of-time conversion :D no need to make it es5, or rather, it's better this way. (less work to do on the es6-classes branch :D )

@neverfox Thanks for the feedbacks, I think we shouldn't worry about hot reloading in this case. these classes are very small. the only place for stateless functional components (I HATE THAT NAME :/ ) are utils like this, examples and very tiny components. all the other are going to be es6-classes. besides, at the worst case it's only a reload :D

@newoga I'll review your code in a moment and provide feedback 😁

neverfox · 2015-12-13T19:55:35Z

@newoga @subjectix Thanks, and I really appreciate the friendly tone around here.

alitaheri · 2015-12-13T19:59:58Z

src/mui-theme-provider.jsx

+  muiTheme: PropTypes.object,
+};
+
+ThemeProvider.childContextTypes = {


can't these (and propTypes) be static members of the ThemeProvider?

Yeah we should be able to do that if we have the right babel stage/preset. I'll give it a try and if it works I'll move it!

yeah, see if it works, if not fall back, take cover :D

When I tried making them static members, the eslinter doesn't detect that muiTheme and children as propTypes, and produces an error...I noticed we're not using the latest versions of some eslint related packages. I can investigate a bit...

if you can't find anything just disable the faulted rules for this file. (with a tiny comment saying why) Thanks a lot.

alitaheri · 2015-12-13T20:02:22Z

src/divider.jsx

@@ -51,7 +42,6 @@ const Divider = ({inset, style, ...other}, {muiTheme = ThemeManager.getMuiTheme(

 Divider.propTypes = propTypes;
 Divider.defaultProps = defaultProps;
-Divider.contextTypes = contextTypes;
-Divider.childContextTypes = childContextTypes;
+Divider = muiThemeable(Divider);


this will have no name, please add a displayName member 😁

newoga · 2015-12-14T01:37:34Z

@subjectix Just letting you know, I haven't abandoned this PR yet, still working on cleaning this up and fixing the issues you highlighted. I had a separate idea/branch relating to this that I decided to present as PR #2509. Feel free to review when it's convenient. Thanks!

newoga · 2015-12-14T02:29:13Z

Alright, I was able to make the propTypes and childContextTypes in mui-theme-provider.jsx as static members. I think my node packages were out of date, I wasn't able to reproduce my issue after updating. I also added displayName to Divider.

Still couldn't get this to work:

export default muiThemeable(Divider);

The error seems to be coming from the parse method from react-docgen. It's possible it doesn't know it might not know how to analyze components that are decorated from an export.

alitaheri · 2015-12-14T06:04:14Z

@newoga I see, then never mind, seems like the const is cursed 😁

P.S. It's all good, thanks a bunch 👍 👍

@oliviertassinari Do a final review on this one.

oliviertassinari · 2015-12-14T12:28:56Z

src/mui-themeable.js

+    return <WrappedComponent {...props} muiTheme={muiTheme} />;
+  };
+
+  MuiComponent.displayName = `mui(${getDisplayName(WrappedComponent)})`;


I'm wondering. Some of our components rely on the displayName to handle composition correctly.
Aren't going to break this by adding the mui(X)?

yup, that's gonna happen! Maybe we shouldn't relay on display name, doesn't instanceof work with es6-classes? is it does I think we should use instanceof instead. hell of a lot safer!

Yeah I'd have to look at how other components are using displayName currently. I'm not sure if this makes a difference, but this function isn't changing the name of the component it's wrapping, it's just changing the name of the high order "wrapping" component to be mui(NameOfWrappedComponent). So the X component still lives in the tree underneath this and is appropriately named X.

I haven't tested this approach beyond the Divider component we merged recently (which is a very simple component). Later in a this branch I'll convert some of the more complex components and those that use displayName to use muiThemeable and investigate the impact.

it's just changing the name of the high order "wrapping" component to be mui(NameOfWrappedComponent)

I think that's the issue. The parent component will just see the HOC one.

Okay, got you. I'm not sure or remember what the use cases are in which components are looking at the displayName of a child component. I'll investigate a bit more. Slowly but surely I'll get a better grasp of the codebase :)

Mm, just to play devil's advocate, for this particular use case, could it make sense to just clone any element that is passed, and pass a prop that dictates that it belongs to the app-bar, and it's up to the icon-element (whether it's a IconMenu, IconButton, or FlatButton to dictate how style change given that it is in an app-bar?

That way if there are new types of components that support being a icon-element, we don't have to add them to the switch condition AppBar.

I don't know if I agree with myself on that, but that could be a strategy to remove coupling between the components.

I'm going to propose this again. Isn't it better to inverse the control? I mean instead of having parents looking for particular children, they provide a contextual interface for the children to subscribe themselves. This will allow arbitrary nesting of related components without breaking logic. I'll provide a Proposal PR, (like the ones @newoga does) to clarify my point. It will be with the DropDownMenu.

@subjectix Sounds good to me. I agree with looking at that approach.

For now, can we set the displayName of hoc component equal to the child component?

Yeah we can definitely do that.

alitaheri · 2015-12-16T06:03:19Z

nice work @newoga. squash please 😁

@oliviertassinari I'm good with this. feel free to merge ^_^

…rough context

newoga · 2015-12-16T06:40:51Z

@oliviertassinari @subjectix Thanks a lot for your feedback and help on this 😃 Just squashed!

Just wanted to let you know:

Configuring the theme using the ThemeProvider component still hasn't been documented. I started looking into it and realized that the customization/themes page might benefit from some refactoring (for example, we may be able to convert a lot of the html/jsx markup on that page to Markdown files, and use the Markdown component that was created). If you want me to document before merging, let me know and I'll change this back to [WIP].
Still looking into the concerns raised in [WIP] Proposal: Parameterized mui-themeable hoc #2509 and I have a few ideas that I'll present in a new PR when I get a chance, but I still think merging this PR first is a good next step.

Thanks!

alitaheri · 2015-12-16T06:45:01Z

@newoga Really nice work! Your contributions to this library are game changing 😎

We should merge this ASAP, so we can migrate the whole library. Documentation comes later.
Couldn't agree more.

@oliviertassinari Thoughts?

oliviertassinari · 2015-12-16T09:49:34Z

@subjectix I agree with you.
I really think that we are going in the right direction.
@newoga Thanks again 👍

Proposal: mui-themeable higher order component

alitaheri · 2015-12-16T10:02:12Z

@newoga Thanks a lot 👍 🎉 🎉

newoga · 2015-12-16T13:36:20Z

No problem! 🎉 😄

alitaheri added PR: Needs Review labels Dec 12, 2015

alitaheri reviewed Dec 13, 2015
View reviewed changes

neverfox mentioned this pull request Dec 13, 2015

[TextField] Initial implementation of optional underline #2476

Merged

alitaheri reviewed Dec 13, 2015
View reviewed changes

newoga mentioned this pull request Dec 14, 2015

[WIP] Proposal: Parameterized mui-themeable hoc #2509

Closed

alitaheri added PR: Review Accepted and removed PR: Needs Review labels Dec 14, 2015

oliviertassinari reviewed Dec 14, 2015
View reviewed changes

Created mui-themeable higher order component to wrap theme passing th…

fa4f480

…rough context

newoga force-pushed the mui-themeable-hoc branch from 13bcdfd to fa4f480 Compare December 16, 2015 06:31

newoga changed the title ~~[WIP] Proposal: mui-themeable higher order component~~ Proposal: mui-themeable higher order component Dec 16, 2015

oliviertassinari added a commit that referenced this pull request Dec 16, 2015

Merge pull request #2493 from newoga/mui-themeable-hoc

c8a5d45

Proposal: mui-themeable higher order component

oliviertassinari merged commit c8a5d45 into mui:master Dec 16, 2015

oliviertassinari mentioned this pull request Dec 18, 2015

[DatePicker] Call setState-muiTheme in componentWillReceiveProps #2579

Merged

mbrookes added this to the 0.15.0 Release milestone Feb 23, 2016

This was referenced May 8, 2016

[Docs] Update the Roadmap #4205

Closed

[Themes] Fix MuiThemeProvider default theme #4229

Merged

This was referenced May 8, 2020

[Snyk] Security upgrade webpack-dev-server from 3.1.14 to 3.11.0 nbv1982/material-ui#30

Open

[Snyk] Security upgrade webpack-dev-server from 3.1.14 to 3.11.0 hzhz/material-ui#6

Open

This was referenced Jul 8, 2020

[Snyk] Security upgrade webpack-dev-server from 3.1.14 to 3.11.0 nbv1982/material-ui#39

Open

[Snyk] Security upgrade webpack-dev-server from 3.1.14 to 3.11.0 hzhz/material-ui#17

Open

snyk-bot mentioned this pull request May 24, 2021

[Snyk] Upgrade webpack-dev-server from 3.1.14 to 3.11.2 maniator/material-ui#6

Open

hzhz mentioned this pull request Mar 9, 2023

[Snyk] Fix for 4 vulnerabilities hzhz/material-ui#79

Open

zannager added the customization: theme Centered around the theming features label Mar 15, 2023

Proposal: mui-themeable higher order component #2493

Proposal: mui-themeable higher order component #2493

Conversation

newoga commented Dec 12, 2015

alitaheri commented Dec 12, 2015

alitaheri commented Dec 12, 2015

newoga commented Dec 12, 2015

newoga commented Dec 12, 2015

alitaheri commented Dec 13, 2015

newoga commented Dec 13, 2015

alitaheri commented Dec 13, 2015

newoga commented Dec 13, 2015

neverfox commented Dec 13, 2015

newoga commented Dec 13, 2015

neverfox commented Dec 13, 2015

newoga commented Dec 13, 2015

newoga commented Dec 13, 2015

neverfox commented Dec 13, 2015

newoga commented Dec 13, 2015

alitaheri commented Dec 13, 2015

neverfox commented Dec 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Dec 14, 2015

newoga commented Dec 14, 2015

alitaheri commented Dec 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri commented Dec 16, 2015

newoga commented Dec 16, 2015

alitaheri commented Dec 16, 2015

oliviertassinari commented Dec 16, 2015

alitaheri commented Dec 16, 2015

newoga commented Dec 16, 2015