Convert StatusBarTileController hierarchy to React #549
Conversation
| }, | ||
| @ObserveModel({ | ||
| getModel: props => props.repository, | ||
| fetchData: async repository => { |
There was a problem hiding this comment.
👍 on optimizing this. Thanks. I'm working on an idea that will make this much easier.
There was a problem hiding this comment.
Ooh! Looking forward to that ✨ The ObserveModel decorator is already so much nicer than wrangling ModelObservers by hand.
|
|
||
| const newBranchEditor = ( | ||
| <div className="github-BranchMenuView-item github-BranchMenuView-editor"> | ||
| <atom-text-editor |
There was a problem hiding this comment.
I know in the past we had issues using the element version of this with Etch. I'm assuming you haven't ran into any issues?
@nathansobo, what was the issue when using this with Etch?
There was a problem hiding this comment.
Huh, it certainly seems to work for me. Is it some issue with registering the TextEditor properly in the workspace and such... ?
There was a problem hiding this comment.
@nathansobo, what was the issue when using this with Etch?
I don't recall a specific issue other than the custom element API is sort of an abandoned effort at the moment. I prefer the more JS-oriented approach of using the component but if this is working for you, then I think it's fine.
lib/views/push-pull-view.js
Outdated
| fetchInProgress: React.PropTypes.bool, | ||
| behindCount: React.PropTypes.number, | ||
| aheadCount: React.PropTypes.number, | ||
| }; |
There was a problem hiding this comment.
Nit: some of these static props use ;, some don't. Shall we standardize on not so as to be consistent? (Surprised eslint doesn't catch it...)
There was a problem hiding this comment.
Oh whoops, good catch. I'll standardize on no semis.
It looks like there's an eslint issue for this: babel/eslint-plugin-babel#43 Still open, but there's already a PR babel/eslint-plugin-babel#121 to add it.
| assert.lengthOf(ts, 1); | ||
| ts[0].show(); | ||
| return ts[0].getTooltipElement(); | ||
| } |
There was a problem hiding this comment.
Happy to entertain suggestions to make this less hard
There was a problem hiding this comment.
Yah, Portal-based components like Tooltip make Enzyme unhappy. One solution from the thread at enzymejs/enzyme#252 is to always use a ref on the element just inside the Portal and manually create a ReactWrapper from the result. I fussed around with that for a while but:
- Because the StatusBarTileController is decorated, we'd need a
refin ObserveModel, too, to access the target component there to get at the real StatusBarTileController and its refs; - I was having trouble getting the ReactWrappers to work at all anyway (they didn't seem to be able to properly find children of the portal'd DOM elements); and
- Putting
refseverywhere feels imperative and un-React-y.
In the end, I settled on using the Atom API to access the root node of the Portal'd DOM subtree and switching over to using DOM traversal APIs within the tooltip content. It is awkward to have half of these tests written with Enzyme helpers and half using DOM APIs directly, but it works pretty well and is still fairly readable with methods like querySelector.
| select.value = value; | ||
|
|
||
| const event = new Event('change', {bubbles: true, cancelable: true}); | ||
| select.dispatchEvent(event); |
There was a problem hiding this comment.
Does Enzyme provide dispatchEvent style methods we can use to simulate events (similar to React's TestUtils)? (Assuming I'm understanding this right....)
There was a problem hiding this comment.
It does! There's a simulate() method on wrappers that can dispatch events. The problem is that I couldn't figure out how to get select to be a ReactWrapper; it's a native DOM element because it's pulled from the tooltip DOM root. So, I fell back to this.
My first stab at part of #381. Convert the
StatusBarTileControllercomponent hierarchy from Etch to React. Along the way, generalize theTooltipcomponent to act likeDecorationand such, and convert all the tests to Enzyme.