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

React: Remove uses of React.createClass in code-studio/components #17624

Merged
merged 31 commits into from Sep 8, 2017

Conversation

islemaster
Copy link
Contributor

More work extracted from #16861 getting us ready to update to React 15.6+. React.createClass is deprecated, and I'm slowly upgrading our components to ES6 classes.

This third batch is components within src/code-studio/components.

constructor(props) {
super(props);
this.state = {
selectedOption: (props.onClickExport && 'export') || 'embed',
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems better expressed as props.onClickExport ? 'export' : 'embed', but I guess that's out of scope for this change.

const top = Math.min(dragStart.row, row),
left = Math.min(dragStart.col, col),
bottom = Math.max(dragStart.row, row),
right = Math.max(dragStart.col, col);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: isnt this too much indenting? should be either 2 or 4 spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Was following the original pattern here, but happy to update.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont really care too much either way, but one could argue that the original pattern is okay because it's 5 spaces, and the latter pattern is not because it's five (though both align with the text from the first line).

if (this.props.serializedMaze) {
cells = this.props.serializedMaze.map(function (row) {
if (props.serializedMaze) {
cells = props.serializedMaze.map(function (row) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could be arrow func, here and line 68

@@ -150,7 +151,7 @@ var GridEditor = React.createClass({
}, this);
}, this);

var serializedData = cells.map(function (row) {
const serializedData = cells.map(function (row) {
return row.map(function (cell) {
return cell.serialize();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

here and lines 145-148 are more places we could use arrow funcs. Not sure how completely you're trying to ES6ify (will prob stop commenting on this if there are other places like it).

};

render() {
const cyan = '#0094ca';
Copy link
Contributor

Choose a reason for hiding this comment

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

neat: a color not in our color file :(

@@ -44,8 +44,8 @@ const style = {
},
};

const AdvancedShareOptions = Radium(React.createClass({
propTypes: {
const AdvancedShareOptions = Radium(class extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but have we considered adding decorator support and using @Radium to add Radium to our components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be awesome. I loved using annotations in AS3.

@islemaster islemaster merged commit cb08e42 into staging Sep 8, 2017
@islemaster islemaster deleted the react-no-createclass-code-studio-components branch September 8, 2017 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants