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

Migrate components to ES6 Classes #458

Closed
rob0rt opened this issue Mar 23, 2015 · 37 comments
Closed

Migrate components to ES6 Classes #458

rob0rt opened this issue Mar 23, 2015 · 37 comments
Labels
customization: css Design CSS customizability

Comments

@rob0rt
Copy link
Contributor

rob0rt commented Mar 23, 2015

Specifically in the new React version, ES6 Classes were introduced, amongst other features. Is there any work being done/ can a branch be opened to start work on this so the peer dependency can be updated? I know they're not dependent on each other but upgrading the library to comply with the new design would be an awesome step forward.

@rob0rt
Copy link
Contributor Author

rob0rt commented Mar 23, 2015

I'd be more than happy to help work on this and migrate components and the docs/ example to the new ES6 Classes and etc.

@DylanPiercey
Copy link

What about mixins?

@rob0rt
Copy link
Contributor Author

rob0rt commented Mar 24, 2015

I'm not sure actually. It appears like the main Mixin to get rid of is Classable which does appear in a lot of different components. Maybe make it a class of its own and extend React.Component from it, and then have each component extend ClassableReactComponent or something?

@mmrtnz
Copy link
Contributor

mmrtnz commented Mar 25, 2015

@BobertForever I would hold off until after our css refactoring is complete, so we can avoid too many merge conflicts. Currently, Classable is used to allow users to apply their custom styles to components. It looks like React will not have mixin support for ES6 classes at least not anytime soon, so we might have to stick with React.createClass for now. We'll need to look more into ES6 classes to see what would be a good work around.

@rob0rt
Copy link
Contributor Author

rob0rt commented Mar 25, 2015

Would creating a Classable ES6 class that extends React.Component work? And then for the components that use only the Classable mixin, just extend Classable instead?
I haven't looked too much into the semantics of React and ES6 classes so I'm not positive this would work but it looks like it might from some preliminary research.

@daniellewissc
Copy link
Contributor

the problem comes when you have more than one mixin, as we will have in many of the components after the css refactoring is done. https://github.com/callemall/material-ui/blob/css-in-js/src/js/mixins/style-propable.js

@rob0rt
Copy link
Contributor Author

rob0rt commented Mar 26, 2015

Hmm... I'm trying to think of a solution to that. There's the chained extends which would get complicated. Another option would be to generalize those things out to the point where you could put them into a utility class and just use them like helper functions.

@gilbox
Copy link

gilbox commented Mar 29, 2015

Just perusing this issue tracker and thought I'd drop my unsolicited 2 cents... here's an approach to es6 mixins

@giltig
Copy link

giltig commented Mar 29, 2015

@yathit
Copy link

yathit commented Mar 31, 2015

I am not fully understand. React router is removing mixin with router context https://github.com/rackt/react-router/blob/master/docs/api/RouterContext.md

@rob0rt
Copy link
Contributor Author

rob0rt commented Mar 31, 2015

@yathit that was actually addressed in a few pull requests recently. Material-UI now uses the new React-router context style, not the mixins.

@hai-cea hai-cea changed the title Update to use new React 0.13.x features / style Migrate components to ES6 Classes Jun 18, 2015
@niln1
Copy link

niln1 commented Aug 17, 2015

There is a react mixin decorator converting mixins to decorators
https://github.com/timbur/react-mixin-decorator/blob/master/index.jsx

@oliviertassinari
Copy link
Member

First attent #1734

@bpmckee
Copy link

bpmckee commented Nov 20, 2015

Is there any update on this?
The docs example still uses the stylePropable mixin.

The actual style-propable mixin says it is unnecessary after v0.11, right now it's on v0.13.x
https://github.com/callemall/material-ui/blob/master/src/mixins/style-propable.js

I'm having all sorts of issues using material-ui with es6 classes. :(

Edit: Please disregard this, I was able to finally follow this guid successfully. My error was due to something else. Sorry.
http://material-ui.com/#/customization/themes

@neverfox
Copy link
Contributor

@bpmckee Does that mean that if you follow that guide, you no longer need StylesPropable? I've been studying the docs example and they use the various mixins all over the place. I'm building a strictly ES6/ES7 project, so I need to know which mixins are still necessary so that I can find a workaround..

@bpmckee
Copy link

bpmckee commented Nov 26, 2015

That's correct. Granted, I'm just getting into the library but I haven't had the need to use any mixins.

For example I could have a component like this (and it'd be styled correctly based on my raw theme).

import React from 'react';
import { AppCanvas, AppBar } from 'material-ui';
// NOTE: import the theme manager like this, apparently doing it any other way grabs the wrong one
import ThemeManager from 'material-ui/lib/styles/theme-manager';
import themeDecorator from 'material-ui/lib/styles/theme-decorator';
import myRawTheme from './myRawTheme';

@themeDecorator(ThemeManager.getMuiTheme(myRawTheme))
export default class MyComponent extends React.Component {
    render() {
        return (
            <AppBar title="Test" />
            <h1>The app bar is styled based on my raw theme</h1>
        );
    }
}

I actually got a lot of this from http://material-ui.com/#/customization/themes which is a lot different than the docs example I was trying to follow when I asked the question initially.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

It's been a while since I've looked at this; would using a Stage 1 decorator on the class to handle the mixin be a valid way to migrate the components over to the new syntax? I'm currently doing that for my React project and I've been loving the new syntax and style. It keeps the code working exactly the same as you have it but allows you to upgrade the syntax. From there you could later deprecate the mixin if that's your end-goal.

I see @bpmckee suggested something similar, but my proposal would make this:

import React from 'react';
import ContextPure from '../mixins/context-pure';
import StylePropable from '../mixins/style-propable';
import DefaultRawTheme from '../styles/raw-themes/light-raw-theme';
import ThemeManager from '../styles/theme-manager';

const FlatButtonLabel = React.createClass({

  mixins: [
    ContextPure,
    StylePropable,
  ],

  contextTypes: {
    muiTheme: React.PropTypes.object,
  },

  propTypes: {
    label: React.PropTypes.node,
    style: React.PropTypes.object,
  },

  //for passing default theme context to children
  childContextTypes: {
    muiTheme: React.PropTypes.object,
  },

  getChildContext() {
    return {
      muiTheme: this.state.muiTheme,
    };
  },

  getInitialState() {
    return {
      muiTheme: this.context.muiTheme ? this.context.muiTheme : ThemeManager.getMuiTheme(DefaultRawTheme),
    };
  },

  //to update theme inside state whenever a new theme is passed down
  //from the parent / owner using context
  componentWillReceiveProps(nextProps, nextContext) {
    let newMuiTheme = nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme;
    this.setState({muiTheme: newMuiTheme});
  },

  statics: {
    getRelevantContextKeys(muiTheme) {
      return {
        spacingDesktopGutterLess: muiTheme.rawTheme.spacing.desktopGutterLess,
      };
    },
  },

  render: function() {
    const {
      label,
      style,
    } = this.props;

    const contextKeys = this.constructor.getRelevantContextKeys(this.state.muiTheme);

    const mergedRootStyles = this.mergeStyles({
      position: 'relative',
      padding: '0 ' + contextKeys.spacingDesktopGutterLess + 'px',
    }, style);

    return (
      <span style={this.prepareStyles(mergedRootStyles)}>{label}</span>
    );
  },

});

export default FlatButtonLabel;

Look like this:

import React from 'react';
import ContextPure from '../mixins/context-pure';
import StylePropable from '../mixins/style-propable';
import DefaultRawTheme from '../styles/raw-themes/light-raw-theme';
import ThemeManager from '../styles/theme-manager';
import { mixin } from 'core-decorators';

@mixin(ContextPure, StylePropable)
export default class FlatButtonLabel extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      muiTheme: this.context.muiTheme
        ? this.context.muiTheme
        : ThemeManager.getMuiTheme(DefaultRawTheme),
    };
  }

  static contextTypes = {
    muiTheme: React.PropTypes.object,
  }

  static propTypes = {
    label: React.PropTypes.node,
    style: React.PropTypes.object,
  }

  //for passing default theme context to children
  static childContextTypes = {
    muiTheme: React.PropTypes.object,
  }

  getChildContext() {
    return {
      muiTheme: this.state.muiTheme,
    };
  }

  //to update theme inside state whenever a new theme is passed down
  //from the parent / owner using context
  componentWillReceiveProps(nextProps, nextContext) {
    let newMuiTheme = nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme;
    this.setState({muiTheme: newMuiTheme});
  }

  static getRelevantContextKeys(muiTheme) {
    return {
      spacingDesktopGutterLess: muiTheme.rawTheme.spacing.desktopGutterLess,
    };
  }

  render() {
    const {
      label,
      style,
    } = this.props;

    const contextKeys = this.constructor.getRelevantContextKeys(this.state.muiTheme);

    const mergedRootStyles = this.mergeStyles({
      position: 'relative',
      padding: '0 ' + contextKeys.spacingDesktopGutterLess + 'px',
    }, style);

    return (
      <span style={this.prepareStyles(mergedRootStyles)}>{label}</span>
    );
  }

}

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

@oliviertassinari and @subjectix, I see you're the most active maintainers as of late; I just wanted to tag you since this issue is kind of old and I'd be interested in doing a lot of the work I suggested above if desired.

@oliviertassinari
Copy link
Member

@BobertForever Well, I think that this work should be done by a code migration tool, as facebook is doing it with his codebase. I'm preaty sure those tools are already available, this shouldn't be too much work.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

Yeah, react-codemod does a lot of the conversion to es6, but not the mixin step. I can run it and fix the mixins and have a PR up?

@alitaheri
Copy link
Member

@BobertForever I'm also a C# developer, so classes all the way 👍 👍 But I think minix are anti patterns all the way too. You can see even the craetors of React bragging about it 😁. We should move those mixin methods to either library functions (a LOT easier to track, debug, reason about, etc) and the tightly coupled ones to HOCs @oliviertassinari Ideas?

@alitaheri
Copy link
Member

@oliviertassinari I'm going to create a milestone to address this. are you ok with it?

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

@subjectix Yeah I agree in regards to the mixins, I'm just proposing an intermediate step while the more difficult work of removing them all together is being considered. I think jumping from having mixins to es6 AND not having mixins is a bit much, especially saying as there is no plan (from what I can tell) to get rid of them in the repo's current state.

@alitaheri
Copy link
Member

@BobertForever It's a lot safer to first remove mixins from the library and then migrate to es6. Because then maybe we can use tools to conver to es6 with comfort.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

After doing some experimentation, the tool provided by Facebook doesn't handle es7 things at all, such as static class properties, and handles context poorly. Regardless, it still got it about 99% of the way there, and moving the propTypes, contextTypes, etc. to es7 static class properties took a few seconds. The mixin was ignored but I just tacked it back onto the class with the decorator I showed in my example above.

All in all the tool did all the work and I just fixed the mixin.

@alitaheri
Copy link
Member

@BobertForever If we remove all the mixins first we can omit decorators and save ourselves from one dependency. @oliviertassinari Thoughts?

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

@subjectix I also used that dependency for the decorator to autobind this, since es6 React classes don't do that for you. You can see it in my PR in src/app-bar.jsx. While this is not necessary, and you can just do the this binding in the constructor, I personally think it's much cleaner to use the decorator.

I will defer to your thoughts, though.

@alitaheri alitaheri added new feature New feature or request and removed Enhancement labels Dec 8, 2015
@alitaheri alitaheri modified the milestones: Migration to ES2015 Classes, 0.16.0 Release Feb 14, 2016
@oliviertassinari
Copy link
Member

That's almost done with #3843 🎉.
We can close it once prefer-es6-classes is enforced.

@oliviertassinari oliviertassinari added Refactoring and removed new feature New feature or request labels Apr 2, 2016
@zannager zannager added the customization: css Design CSS customizability label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: css Design CSS customizability
Projects
None yet
Development

No branches or pull requests