Improve search results markup and add tests #62
Conversation
| @@ -7,24 +7,30 @@ const render = ReactTestUtils.renderIntoDocument; | |||
| const findByTag = ReactTestUtils.findRenderedDOMComponentWithTag; | |||
|
|
|||
| describe('<SearchForm />', () => { | |||
| var onSearch; | |||
There was a problem hiding this comment.
Doesn't really matter here but should we always prefer let?
There was a problem hiding this comment.
We could - I'm on the fence about that. I'm a bit too used to using var and I've only been using let when I need it. Maybe we should just use let and const and be done with it? Happy to hear thoughts on that - I can go with the consensus and adapt.
There was a problem hiding this comment.
To me it seems like let is just var with less footguns. So we should always use let and const.
There was a problem hiding this comment.
Yep fair enough - I'll move us to let + const. I might do it separate to this patch though since it'll be easier to review a patch that updates everything in one hit.
muffinresearch
force-pushed
the
fix-markup
branch
from
a974008
to
0f998bb
Compare
| }); | ||
|
|
||
| it('calls onSearch with a search query', () => { | ||
| const onSearch = sinon.spy(); | ||
| const root = render(<SearchForm onSearch={onSearch} />); | ||
| root.refs.query.value = 'adblock'; |
There was a problem hiding this comment.
The ref value is query or are you suggesting changing it?
There was a problem hiding this comment.
beforeEach has input = root.refs.query so this could be input.value instead.
There was a problem hiding this comment.
Ah I see. Yep I'll update it.
| it('renders error when query is an empty string', () => { | ||
| const root = renderResults({query: ''}); | ||
| const searchResultsMsg = root.refs.message; | ||
| assert.include(searchResultsMsg.firstChild.nodeValue, 'supply a valid search'); |
There was a problem hiding this comment.
Should this use textContent like the others?
There was a problem hiding this comment.
Doesn't need to since there's no active content so no implicit spans added by react.
|
r+wc |
muffinresearch
force-pushed
the
fix-markup
branch
from
5be39a6
to
5db80a9
Compare
muffinresearch
force-pushed
the
fix-markup
branch
from
5db80a9
to
e529a36
Compare
Improve search results markup and add tests
I'll file some issues for DRYing up the things we need in tests. Would be nice if we could have most of ReactTestUtils aliased in by default. Also I'll add and issues for adding l10n features.