fix(search-form): extract search actions component #3475
Conversation
| @@ -41,13 +24,13 @@ type Props = { | |||
| method?: 'get' | 'post', | |||
| /** Name of the text input */ | |||
| name?: string, | |||
| /** On change handler for the search input, debounced by 250ms */ | |||
There was a problem hiding this comment.
outdated comment - the debounce was removed in box-react-ui
| </div> | ||
| ); | ||
|
|
||
| const LoadableSearchActions = makeLoadable(SearchActions); |
There was a problem hiding this comment.
legacy docs state to not use HOCs in the render method: https://legacy.reactjs.org/docs/higher-order-components.html#dont-use-hocs-inside-the-render-method
I don't know if this is the direct cause of the original issue but it ended up fixing it without changing the other logic
| // Sift through the nested HOCs to find the correct element | ||
| const searchActions = wrapper | ||
| .find('LoadableSearchActions') | ||
| .shallow() | ||
| .shallow() | ||
| .shallow(); |
There was a problem hiding this comment.
admittedly not my best work...I briefly looked into using mount and alternatives but I selfishly didn't want to spend too much time on this component and changes
There was a problem hiding this comment.
shouldn't we being use two dive()s here instead of two shallow()?
There was a problem hiding this comment.
Also should this test be here? shouldn't it be in a new test file for SearchActions this way you dont have to go through the HOC?
There was a problem hiding this comment.
shouldn't we being use two dive()s here instead of two shallow()?
I mainly copied from the existing test since I wasn't sure the exact differences. but it looks like dive() is preferred so I'll update these:
enzymejs/enzyme#1798
There was a problem hiding this comment.
Also should this test be here? shouldn't it be in a new test file for SearchActions this way you dont have to go through the HOC?
yeah this was one of the alternatives I was looking into but then moving it to a new SearchActions file I think sort of defeats the intention of the original test since it's testing the onClearHandler in SearchForm (the parent component)
There was a problem hiding this comment.
I think the ideal way to test it would be to convert this to RTL I guess and test the button click functionality? but then I was like: ahhh we'll save that for another day...
| // Sift through the nested HOCs to find the correct element | ||
| const searchActions = wrapper | ||
| .find('LoadableSearchActions') | ||
| .shallow() | ||
| .shallow() | ||
| .shallow(); |
There was a problem hiding this comment.
shouldn't we being use two dive()s here instead of two shallow()?
| // Sift through the nested HOCs to find the correct element | ||
| const searchActions = wrapper | ||
| .find('LoadableSearchActions') | ||
| .shallow() | ||
| .shallow() | ||
| .shallow(); |
There was a problem hiding this comment.
Also should this test be here? shouldn't it be in a new test file for SearchActions this way you dont have to go through the HOC?
| onClear: (event: React.SyntheticEvent<HTMLButtonElement>) => void; | ||
| } | ||
|
|
||
| const SearchActions = ({ hasSubmitAction, intl, onClear }: SearchActionsProps) => { |
There was a problem hiding this comment.
fyi this component is just copied from SearchForm. leaving class names and whatnot as the same so it doesn't break existing functionality for apps
This should address an issue in the features version of ContentExplorer where passing a
searchInputPropsprop prevented the search input to not submit. I honestly couldn't figure out the exact root cause but extracting themakeLoadableHOC out of therendermethod seemed to fix it.