Convert remaining components to functional components #464
Conversation
Co-authored-by: Hrusikesh Panda <[email protected]>
(cherry picked from commit 3cf90bc)
(cherry picked from commit 2701aa2)
…rface (dowjones#434) (cherry picked from commit 3db87c9)
Co-authored-by: Marian Juhas <[email protected]> (cherry picked from commit 2212020)
| const keepDropdownActive = useRef(false) | ||
| const callbackRef = useRef() | ||
|
|
||
| const setStateWithCallback = (newState, callback) => { |
There was a problem hiding this comment.
I created it to emulate the callback arg in the old setState. In the future, this component can be refactored to not need it.
Pull Request Test Coverage Report for Build 1533
💛 - Coveralls |
|
This is awesome work @m4theushw !! Thanks for sending this. I'm gonna go over it this weekend and having the long weekend helps. This is the first step towards having headless components and I'm glad we're heading in that direction. |
| @@ -29,7 +29,7 @@ test('some key presses opens dropdown on keyboardNavigation', t => { | |||
| ;['ArrowUp', 'ArrowDown', 'Home', 'PageUp', 'End', 'PageDown', 'a', 'B'].forEach(key => { | |||
| const wrapper = mount(<DropdownTreeSelect data={tree} />) | |||
| triggerOnKeyboardKeyDown(wrapper, key) | |||
| t.true(wrapper.state().showDropdown) | |||
| t.true(wrapper.exists('.dropdown-content')) | |||
There was a problem hiding this comment.
I vaguely remember having this kind of check which wasn't accurately detecting the dropdown state. state.showDropdown was more trusty as it gets set when the event handler gets called indicating the dropdown is being shown indeed. The only part trust in that case is the CSS properties actually render the div visible but we don't need to test the browser's CSS engine, so it was an acceptable compromise.
There was a problem hiding this comment.
We could check if the role is "listbox" or "tree". That would be a more recommended approach if React Testing Library was used.
There was a problem hiding this comment.
Alright. So let's continue with this and tackle it later if we run into similar issues. Maybe moving to RTL could be another PR?
There was a problem hiding this comment.
Yes, dropping enzyme in favor of RTL would be a great improvement in testing quality.
| wrapper.find('.search').simulate('click') | ||
| t.true(wrapper.exists('.dropdown-content')) | ||
| const event = new MouseEvent('click', { bubbles: true, cancelable: true }) | ||
| global.document.dispatchEvent(event) |
There was a problem hiding this comment.
This is interesting. So with this dispatch, do we still need the simulate('click')? Seems like they are both asserting the same thing?
There was a problem hiding this comment.
I kept the simulate('click') because the old test had it, but it could be removed. The purpose of the dispatch is to ensure that, if the user clicks outside the dropdown, it stays open.
|
|
||
| const searchInput = ( | ||
| <Input | ||
| ref={searchInputRef} |
There was a problem hiding this comment.
I had some problems passing the ref as inputRef. It seems like AVA can't deal with refs passed through custom named props.
There was a problem hiding this comment.
Trying to understand here - don't we do that already? What was the exact error/issue you ran into?
There was a problem hiding this comment.
The error I got when using inputRef is:
PointerLookupError {
index: 10,
message: 'Could not deserialize buffer: pointer 10 could not be resolved',
}
I found this relatated issue. I tried to upgrade AVA to the latest version but it didn't solve the problem.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions. |
What does it do?
Checklist: