Refactor some of the item components #1983
Conversation
…eneral item component
Codecov Report
@@ Coverage Diff @@
## master #1983 +/- ##
=========================================
- Coverage 92.12% 92.1% -0.02%
=========================================
Files 188 189 +1
Lines 10807 10758 -49
Branches 1581 1575 -6
=========================================
- Hits 9956 9909 -47
+ Misses 851 849 -2
Continue to review full report at Codecov.
|
vanessayuenn
changed the title
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
The autobind thing is the only actual "we should change this before 🚢" issue I found - other than that it's all comments and discussion. Thanks for tackling this, the duplication has been bugging me 😆
| import {Emitter} from 'event-kit'; | ||
| import {autobind} from '../helpers'; | ||
|
|
||
| export default class ItemComponent extends React.Component { |
There was a problem hiding this comment.
This feels un-React-y to me, but I'm not 100% sure what the alternative would look like in this case.
From https://reactjs.org/docs/react-component.html#overview:
We strongly recommend against creating your own base component classes. In React components, code reuse is primarily achieved through composition rather than inheritance.
...and they link to the guide about favoring composition over inheritance.
The way we use React is sufficiently different from the uses that the guide is written for that I'm not totally opposed to this. But the angry bolded warning is enough to make me want to at least spitball some possible alternatives first 😆 Without actually trying to implement them, I can think of three off the top of my head.
- Wrapper component
We could create an Item component that implements Item-level concerns directly, configured via props, and renders children:
<PaneItem
workspace={this.props.workspace}
uriPattern={GitHubTabItem.uriPattern}
className="github-GitHub-root">
{({itemHolder}) => (
<Item title="GitHub" icon="octoface" needsDestroy={false} needsPending={false}>
<GitHubTabItem
ref={itemHolder.setter}
repository={this.props.repository}
loginModel={this.props.loginModel}
workspace={this.props.workspace}
/>
</Item>
)}
</PaneItem>- Higher-order component
We could create a higher-order component that wraps Items:
import {createItem} from '../../atom/item';
class GithubTabItem extends React.Component {
// ...
}
export default createItem(GithubTabItem, {
title: 'GitHub',
icon: 'octoface',
});- Roll it into
PaneItem
We could manage item-level concerns entirely within PaneItem.
<PaneItem
workspace={this.props.workspace}
uriPattern={GitHubTabItem.uriPattern}
className="github-GitHub-root"
title="GitHub"
icon="octoface"
needsDestroy={false}
needsPending={false}>
{({itemHolder}) => (
<GitHubTabItem
ref={itemHolder.setter}
repository={this.props.repository}
loginModel={this.props.loginModel}
workspace={this.props.workspace}
/>
)}
</PaneItem>
What do you think about these? Did you already consider these and decide against them?
There was a problem hiding this comment.
I think you're right in that inheritance is inherently (heh) not very react-y. Upon some investigation, I think wrapper component but implemented with render prop pattern might be the best way to go, especially since we have quite some existing render prop patterns in the codebase already.
|
|
||
| constructor(props, {title, icon}) { | ||
| super(props); | ||
| autobind(this, 'destroy'); |
There was a problem hiding this comment.
🚨 Rather than using autobind, we should prefer the class-property syntax:
class ItemComponent extends React.Component {
// ...
destroy = () => {
// ...
}
}I've got a PR open to get rid of autobind() entirely (#1850) so we should keep it out of new code if possible.
| } | ||
| }); | ||
|
|
||
| describe('destroy and terminatePendingState should be overridden as no-ops', function() { |
There was a problem hiding this comment.
🤔 What happens if these aren't overridden as no-ops?
|
Closing this out for now until we figure out a better approach for the refractor :) |
Resolves #1923. I am sure there is stuff that can be further refactored, but I didn't want to overdo it. Thought this would be a good start.