ES6 AppView.jsx #22027
Merged
ES6 AppView.jsx #22027
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maddiedierker
approved these changes
Apr 25, 2018
| onMount: () => {}, | ||
| }; | ||
|
|
||
| it('no class names', () => { |
There was a problem hiding this comment.
This is very small, but I might go into a little more detail in your it statements (this test and below). I wasn't sure what each test was highlighting until I read through all of the expects. However, the expects themselves might do a better job than trying to boil down something descriptive enough for each it statement, so not sure that those need to be changed right now.
Approved, and this looks great! 🎉 yay ES6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have a slow ongoing process of replacing all our uses of
React.createClasswith ES6 classes that inherit fromReact.Component. It's one of the last bits of tech debt we need to address before we can switch to React 16. There's a list of remaining components needing conversion here.I'm just converting one more component with some minimal tests since there were none before. This one was imported with
require()all over the place, so I had to update those imports to support a non-default export from the module (for testing purposes).