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/pd/workshop_dashboard #17612

Closed
wants to merge 34 commits into from

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Sep 8, 2017

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 second batch is components within src/code-studio/pd/workshop_dashboard. I've never made changes in this area, and could especially use help verifying the changes to contextTypes don't mess with any of the react-router behaviors.

@islemaster islemaster force-pushed the react-no-createclass-worshop_dashboard branch from 80e317e to c9cc54c Compare September 8, 2017 17:14
@islemaster islemaster changed the title [WIP] React: Remove uses of React.createClass in code-studio/pd/workshop_dashboard React: Remove uses of React.createClass in code-studio/pd/workshop_dashboard Sep 8, 2017
Copy link
Contributor

@mehalshah mehalshah left a comment

Choose a reason for hiding this comment

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

What's the general urgency on this change? I ask because our testing in this area still has a ways to go - and if something is broken it's going to hard to find.

Have you been able to bring up the workshop dashboard and view workshops in various states? If not, I can show you how

@islemaster
Copy link
Contributor Author

Non-urgent, just doing some code cleanup to get us ready for a React upgrade. I'd appreciate a walkthrough sometime this week on how to manually test workshop dashboard, and I'm also more than happy to break this up into smaller PRs.

Copy link
Contributor

@aoby aoby left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking this on, Brad!

I'm happy to spend some time this week walking through the basic workshop dashboard scenarios with you to make sure this didn't break anything. And yeah, we know we're quite low on test coverage in this space :(

@aoby
Copy link
Contributor

aoby commented Sep 26, 2017

No need to break up into smaller PRs. I would like to walk through some scenarios and make sure we didn't break anything before merging though.

@islemaster
Copy link
Contributor Author

We've got some merge conflicts and some of my PRs from before my vacation caused some regressions too, so I am actually going to break this up into smaller PRs. I'll open the first momentarily.

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