Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(search-form): extract search actions component #3475

Merged
merged 3 commits into from
Dec 14, 2023
Merged

fix(search-form): extract search actions component #3475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a01c2b7
fix(search-form): extract search actions component
tjuanitas Dec 14, 2023
92e4fd5
fix(search-form): use dive over shallow in test
tjuanitas Dec 14, 2023
6353822
Merge branch 'master' into fix-extract-search-actions
mergify[bot] Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/components/loading-indicator/makeLoadable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ export interface MakeLoadableProps {
loadingIndicatorProps?: LoadingIndicatorProps;
}

const makeLoadable = <P extends MakeLoadableProps>(
BaseComponent: React.ComponentType<P>,
): React.FC<P & MakeLoadableProps> => {
const LoadableComponent = ({ isLoading = false, loadingIndicatorProps = {}, ...rest }: MakeLoadableProps) =>
const makeLoadable = <P extends object>(BaseComponent: React.ComponentType<P>): React.FC<P & MakeLoadableProps> => {
const LoadableComponent = ({ isLoading = false, loadingIndicatorProps = {}, ...rest }: P & MakeLoadableProps) =>
isLoading ? <LoadingIndicator {...loadingIndicatorProps} /> : <BaseComponent {...(rest as P)} />;

LoadableComponent.displayName = `Loadable${BaseComponent.displayName || BaseComponent.name || 'Component'}`;
Expand Down
50 changes: 50 additions & 0 deletions src/components/search-form/SearchActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as React from 'react';
import { injectIntl, IntlShape } from 'react-intl';

import ClearBadge16 from '../../icon/fill/ClearBadge16';
import Search16 from '../../icon/fill/Search16';
import makeLoadable from '../loading-indicator/makeLoadable';

import messages from './messages';

export interface SearchActionsProps {
/** Whether to render an interactive search button */
hasSubmitAction: boolean;
/** Intl object */
intl: IntlShape;
/** Called when clear button is clicked */
onClear: (event: React.SyntheticEvent<HTMLButtonElement>) => void;
}

const SearchActions = ({ hasSubmitAction, intl, onClear }: SearchActionsProps) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { formatMessage } = intl;

return (
<div className="action-buttons">
{hasSubmitAction ? (
<button
type="submit"
className="action-button search-button"
title={formatMessage(messages.searchButtonTitle)}
>
<Search16 />
</button>
) : (
<div className="action-button search-button">
<Search16 />
</div>
)}
<button
className="action-button clear-button"
onClick={onClear}
title={formatMessage(messages.clearButtonTitle)}
type="button"
>
<ClearBadge16 />
</button>
</div>
);
};

export { SearchActions as SearchActionsBase };
export default makeLoadable(injectIntl(SearchActions));
64 changes: 10 additions & 54 deletions src/components/search-form/SearchForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,17 @@
// @flow
import * as React from 'react';
import { defineMessages, injectIntl } from 'react-intl';
import { injectIntl } from 'react-intl';
import classNames from 'classnames';
import omit from 'lodash/omit';

import ClearBadge16 from '../../icon/fill/ClearBadge16';
import Search16 from '../../icon/fill/Search16';
// $FlowFixMe
import SearchActions from './SearchActions';

import makeLoadable from '../loading-indicator/makeLoadable';
// $FlowFixMe
import messages from './messages';

import './SearchForm.scss';

const messages = defineMessages({
clearButtonTitle: {
defaultMessage: 'Clear',
description: 'Title for a clear button',
id: 'boxui.searchForm.clearButtonTitle',
},
searchButtonTitle: {
defaultMessage: 'Search',
description: 'Title for a search button',
id: 'boxui.searchForm.searchButtonTitle',
},
searchLabel: {
defaultMessage: 'Search query',
description: 'Label for a search input',
id: 'boxui.searchForm.searchLabel',
},
});

type Props = {
/** Form submit action */
action?: string,
Expand All @@ -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 */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outdated comment - the debounce was removed in box-react-ui

/** On change handler for the search input */
onChange?: Function,
/** On submit handler for the search input */
onSubmit?: Function,
/** Extra query parameters in addition to the form data */
queryParams: { [arg: string]: string },
/** Boolean to prevent propogation of search clear action */
/** Whether to prevent propagation of search clear action */
shouldPreventClearEventPropagation?: boolean,
/** If the clear button is shown when input field is not empty */
useClearButton?: boolean,
Expand Down Expand Up @@ -177,35 +160,6 @@ class SearchFormBase extends React.Component<Props, State> {
<input key={index} name={param} type="hidden" value={queryParams[param]} />
));

const SearchActions = () => (
<div className="action-buttons">
{onSubmit ? (
<button
type="submit"
className="action-button search-button"
title={formatMessage(messages.searchButtonTitle)}
>
<Search16 />
</button>
) : (
<div className="action-button search-button">
<Search16 />
</div>
)}

<button
className="action-button clear-button"
onClick={this.onClearHandler}
title={formatMessage(messages.clearButtonTitle)}
type="button"
>
<ClearBadge16 />
</button>
</div>
);

const LoadableSearchActions = makeLoadable(SearchActions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// @NOTE Prevent errors from React about controlled inputs
const onChangeStub = () => {};

Expand All @@ -229,11 +183,13 @@ class SearchFormBase extends React.Component<Props, State> {
type="search"
{...inputProps}
/>
<LoadableSearchActions
<SearchActions
hasSubmitAction={!!onSubmit}
isLoading={isLoading}
loadingIndicatorProps={{
className: 'search-form-loading-indicator',
}}
onClear={this.onClearHandler}
/>
{hiddenInputs}
</form>
Expand Down
14 changes: 9 additions & 5 deletions src/components/search-form/__tests__/SearchForm.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('components/search-form/SearchForm', () => {
onSubmitMock.mockReset();
});

test('should call onsubmit on form submit event', () => {
test('should call onSubmit on form submit event', () => {
const wrapper = mount(<SearchForm onSubmit={onSubmitMock} placeholder="search" value="cheese" />);
const form = wrapper.find('form');

Expand All @@ -75,7 +75,7 @@ describe('components/search-form/SearchForm', () => {
expect(onSubmitMock).toBeCalledWith('cheese', expect.objectContaining(event));
});

test('should call onsubmit on search icon button submit event', () => {
test('should call onSubmit on search icon button submit event', () => {
const wrapper = mount(<SearchForm onSubmit={onSubmitMock} placeholder="search" value="cheese" />);
const searchButton = wrapper.find('.search-button');
expect(searchButton.prop('type')).toEqual('submit');
Expand All @@ -86,7 +86,7 @@ describe('components/search-form/SearchForm', () => {
});
});

test('should call onchange on form change', () => {
test('should call onChange on form change', () => {
const onChangeSpy = sinon.spy();
const wrapper = mount(<SearchForm onChange={onChangeSpy} placeholder="search" value="cheese" />);
const form = wrapper.find('form');
Expand Down Expand Up @@ -124,8 +124,12 @@ describe('components/search-form/SearchForm', () => {

test('should set the onClearHandler to the clear button onClick prop', () => {
const wrapper = shallow(<SearchForm intl={intlShape} />).shallow();
const lodableComponent = wrapper.find('LoadableSearchActions').shallow();
const searchActions = lodableComponent.shallow();
// Sift through the nested HOCs to find the correct element
const searchActions = wrapper
.find('LoadableSearchActions')
.shallow()
.shallow()
.shallow();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we being use two dive()s here instead of two shallow()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@tjuanitas tjuanitas Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

const { onClearHandler } = wrapper.instance();
expect(searchActions.find('.clear-button').prop('onClick')).toEqual(onClearHandler);
});
Expand Down
21 changes: 21 additions & 0 deletions src/components/search-form/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { defineMessages } from 'react-intl';

const messages = defineMessages({
clearButtonTitle: {
defaultMessage: 'Clear',
description: 'Title for a clear button',
id: 'boxui.searchForm.clearButtonTitle',
},
searchButtonTitle: {
defaultMessage: 'Search',
description: 'Title for a search button',
id: 'boxui.searchForm.searchButtonTitle',
},
searchLabel: {
defaultMessage: 'Search query',
description: 'Label for a search input',
id: 'boxui.searchForm.searchLabel',
},
});

export default messages;