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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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
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')
.dive()
.dive()
.dive();
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
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;