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

ES6ify Instructions components #23689

Merged
merged 5 commits into from Jul 13, 2018
Merged

ES6ify Instructions components #23689

merged 5 commits into from Jul 13, 2018

Conversation

islemaster
Copy link
Contributor

Removes React.createClass from InstructionsWithWorkspace and InstructionsDialogWrapper, while adding test coverage for each.

Copy link

@epeach epeach left a comment

Choose a reason for hiding this comment

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

Looks good. I've got a few questions, but overall, it's nice to see more ES6 happening!

@@ -12,43 +12,40 @@ var instructions = require('../../redux/instructions');
* Owns maxHeightAvailable for instructions, updating as appropriate on window
* resize events
*/
var InstructionsWithWorkspace = React.createClass({
propTypes: {
export class UnwrappedInstructionsWithWorkspace extends React.Component {
Copy link

Choose a reason for hiding this comment

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

With this name change, are the places that import this function going to need to be updated?

Copy link

Choose a reason for hiding this comment

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

Also, what's the benefit of adding 'unwrapped' to the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default export for this file (which is unchanged by this PR) is at the end of the file, where we use react-redux's connect method to generate a new, "wrapped" version of the component that is the preferred way to use it. (See Radium for another example of wrapped/higher-order components.)

export default connect(function propsFromStore(state) {
return {
instructionsHeight: state.instructions.renderedHeight
};
}, function propsFromDispatch(dispatch) {
return {
setInstructionsMaxHeightAvailable(maxHeight) {
dispatch(setInstructionsMaxHeightAvailable(maxHeight));
}
};
})(UnwrappedInstructionsWithWorkspace);

Changing the name of the unwrapped component doesn't require us to update any of the places that import this file because default exports are unnamed - the importing file doesn't know or care what the export was named, behind the scenes it's just 'default' and can be renamed on import.

// InstructionsWithWorkspace.jsx
export default class InstructionsWithWorkspace {...}

// Consumer.jsx
import FruitSalad from './InstructionsWithWorkspace'; // works fine!

You can read more about the difference between default and named exports on the MDN page for the export keyword.

Testing Redux-connected components can require quite a bit of setup, though, so here I'm making the unwrapped version of the component a second, non-default export for unit testing purposes. The unit tests import and rename the unwrapped component:

import {
UnwrappedInstructionsWithWorkspace as InstructionsWithWorkspace
} from '@cdo/apps/templates/instructions/InstructionsWithWorkspace';

I could actually get away with not adding "Unwrapped" to the name, but it can lead to some confusion because there's a subtle distinction in how you import it:

// InstructionsWithWorkspace.jsx
export class InstructionsWithWorkspace {...}
export default connect(...)(InstructionsWithWorkspace);

// Consumer.jsx
// import wrapped version
import InstructionsWithWorkspace from './InstructionsWithWorkspace';
// import unwrapped version
import {InstructionsWithWorkspace} from './InstructionsWithWorkspace';

...so I prefer to add something to the name making it obviously the unwrapped version.

Copy link

Choose a reason for hiding this comment

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

Awesome! Thanks for the info!

@islemaster islemaster merged commit faf4f2f into staging Jul 13, 2018
@islemaster islemaster deleted the es6-instructions branch July 13, 2018 20:00
islemaster added a commit that referenced this pull request Jul 14, 2018
islemaster added a commit that referenced this pull request Jul 16, 2018
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

2 participants