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] update components to es6 classes #3843

Merged
merged 10 commits into from
Apr 1, 2016

Conversation

nathanmarks
Copy link
Member

es6 class codemod update

With some inspiration from @oliviertassinari I was able to hack together a quick fork of react-codemod with a class converter that would enable some more es7 features (supported via babel, from the same proposal as the static properties).

Please check out the diff and point out anything that is screwed up! :+1

Also, I could not convert Snackbar because of the mixin it is using (resizable style something or other...). Hopefully that with an improved solution for styling the mixin will become obsolete anyways.


  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@nathanmarks nathanmarks changed the title [WIP][Core] es6 class codemod [WIP][Core] update components to es6 classes Mar 30, 2016
@nathanmarks nathanmarks force-pushed the es6-class-codemod branch 3 times, most recently from 1d0be0e to c0403a4 Compare March 30, 2016 15:20
@nathanmarks
Copy link
Member Author

@callemall/material-ui

One issue is that we currently depend on displayName for type checking. Classes don't have a displayName by default, you need to add it manually or use a babel transform to add it.

Should we do that for now? Or consider #3163 a hard dependency/blocker for this?

(btw, there is an eslint react plugin rule for enforcing displayName, feels gross though 😟 )

@newoga
Copy link
Contributor

newoga commented Mar 30, 2016

@nathanmarks My vote is use a babel transform if possible so that we can merge this (however we should still continue to discuss #3163).

@nathanmarks
Copy link
Member Author

@newoga Want to check it out? I added that transform

@oliviertassinari
Copy link
Member

@nathanmarks I'm definitely not convinced by the https://github.com/nathanmarks/babel-plugin-transform-react-es6-displayname/blob/master/index.js babel-plugin.
Using this plugin is definitely going to increase the size of the lib.
I would rather use the solution I have proposed before.

Few components need a static muiName property. That's going to be more explicit.
I think that the displayName is a debugging util, nothing more.
It feels wrong relying on it to make our components works.

@nathanmarks
Copy link
Member Author

@oliviertassinari I will remove the plugin and manually add static muiName to the components that need type checking. Sounds good? 👍

@oliviertassinari
Copy link
Member

@nathanmarks Awesome 👍.

@nathanmarks nathanmarks changed the title [WIP][Core] update components to es6 classes [Core] update components to es6 classes Apr 1, 2016

componentWillMount() {
this.setState({
date: this._isControlled() ? this._getControlledDate() : this.props.defaultDate,
Copy link
Contributor

Choose a reason for hiding this comment

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

isControlled() instead of _isControlled(), getControlledDate() too

};
},
state = {
inner: isInner(this.props),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

leaving for now as per our convo

@nathanmarks
Copy link
Member Author

@callemall/material-ui need some more eyes giving this a review 👍

@mbrookes
Copy link
Member

mbrookes commented Apr 1, 2016

Tested every component in the docs site, and looks good here, (bar known-issues that are not related to this PR).

@nathanmarks
Copy link
Member Author

@mbrookes whoo! awesome, thanks for reviewing 👍

@newoga
Copy link
Contributor

newoga commented Apr 1, 2016

Awesome stuff @nathanmarks!

@newoga newoga merged commit 0e356c8 into mui:master Apr 1, 2016
@oliviertassinari
Copy link
Member

@nathanmarks Awesome 💯.

@nathanmarks nathanmarks deleted the es6-class-codemod branch April 1, 2016 23:10
@nathanmarks
Copy link
Member Author

Thanks guys! Glad we got this in and can push on now 👍

@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants