React: More templates/progress components converted to ES6 #18525
Conversation
| @@ -58,12 +58,12 @@ const styles = { | |||
| /** | |||
| * A set of one or more levels that are part of the same progression | |||
| */ | |||
| const ProgressLevelSet = React.createClass({ | |||
| propTypes: { | |||
| export default Radium(class ProgressLevelSet extends React.Component { | |||
There was a problem hiding this comment.
is this kind of inlining a new best practice? I thought the export default Radium(ProgressLevelSet); you removed was somewhat more standard in our codebase, but I see now there are a few instances of this new pattern cropping up.
There was a problem hiding this comment.
I picked this up after I noticed Paul doing it. I like it for a couple of reasons:
- I prefer the inline export for the component when it's convenient to do so - I just find it more scannable.
- A component designed with Radium features in mind should probably never be used without Radium.
On the other hand, @Bjvanminnen pointed out that this isn't a great pattern for react-redux's connect because it's fairly common for us to export both a connected and unconnected version of a component - and it's much harder to read. I'd be interested to hear his thoughts here.
There was a problem hiding this comment.
I don't love the readability of this. There's a lot going on here:
export default Radium(class ProgressLevelSet extends React.Component {It gets marginally better if we import Component from React instead, though still not great
import { Component } from 'react';
export default Radium(class ProgressLevelSet extends Component {I think the thing I like least is that the class name gets buried in the middle. Even something like this I would prefer
const ProgressLevelSet = Radium(class extends Component {but that doesnt help us because we can't then do export default on that same line
If we were using ES7 decorators (something I've been somewhat resistant to thus far), this would be improved as well I think
@Radium
export default class ProgressLevelSet extends Component {In general, I think my choice would be to stick the Radium at the end of the file, but I don't think I care as much here as I do for connect, just because the call to Radium is much simpler than everything we pass to connect
There was a problem hiding this comment.
If this is not a pattern we've already accepted, then my vote would be to keep the export default Radium(ProgressLevelSet) on a separate line. This prioritizes clarity above other concerns, whereas the export default Radium(class ... seems like too many different things crammed into one line.
I do like export default Radium(class ... From a "pit of success" perspective, because you can't forget to use Radium when you export e.g. an Unconnected component. The const ProgressLevelSet = Radium(class extends Component { syntax also accomplishes this. However, I don't think this will be a super frequent problem.
In case anyone's curious, here are the only places we use connect, Radium, and export and Unconnected component all within the same file:
There was a problem hiding this comment.
Oops, I sent that last comment a moment early. Disclaimers: I don't think the syntax shown in this PR is particularly heinous, so I'm fine with doing it this way or generally allowing both if those seem easier than trying to have a standard for this.
Thanks for doing all this ES6ifying, Brad!
There was a problem hiding this comment.
It looks like Radium(class only happens in 24 places right now. I see the argument for less on a line. If you both prefer that I'll update this PR and do some followup cleanup of other places we've done this.
| </div> | ||
| ); | ||
| }); | ||
| }); |
More of these... we queue a long time for OSX containers on Travis 馃榿.