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: fix selectPicker when search is controlled and exit Dropdown wit… #2411

Merged
merged 7 commits into from Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/SelectPicker/SelectPicker.tsx
Expand Up @@ -83,7 +83,7 @@ export interface SelectProps<T> {
onGroupTitleClick?: (event: React.SyntheticEvent) => void;

/** Called when searching */
onSearch?: (searchKeyword: string, event: React.SyntheticEvent) => void;
onSearch?: (searchKeyword: string, event?: React.SyntheticEvent) => void;

/** Called when clean */
onClean?: (event: React.SyntheticEvent) => void;
Expand Down Expand Up @@ -288,6 +288,7 @@ const SelectPicker = React.forwardRef(
const handleExited = useCallback(() => {
setSearchKeyword('');
setActive(false);
onSearch?.('');
onClose?.();
}, [onClose, setSearchKeyword]);
Copy link
Member

Choose a reason for hiding this comment

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

React Hook useCallback has a missing dependency: 'onSearch'. Either include it or remove the dependency array

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah,I added it.


Expand Down
57 changes: 55 additions & 2 deletions src/SelectPicker/test/SelectPickerSpec.js
@@ -1,5 +1,5 @@
import React from 'react';
import { render } from '@testing-library/react';
import React, { useRef, useState } from 'react';
import { render, cleanup, fireEvent } from '@testing-library/react';
import ReactTestUtils from 'react-dom/test-utils';
import { getDOMNode, getInstance } from '@test/testUtils';
import SelectPicker from '../SelectPicker';
Expand All @@ -24,6 +24,10 @@ const data = [
}
];

afterEach(() => {
cleanup();
});

Copy link
Member

Choose a reason for hiding this comment

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

cleanup() is unnecessary here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah,I removed it

describe('SelectPicker', () => {
it('Should clean selected default value', () => {
const instance = getDOMNode(<SelectPicker defaultOpen data={data} defaultValue={'Eugenia'} />);
Expand Down Expand Up @@ -400,4 +404,53 @@ describe('SelectPicker', () => {
expect(getByTestId('content')).to.have.text('Not selected');
});
});
it('SearchWord should be reset when controlled and triggered off', done => {
let searchRef = '';
let onClose = null;
const promise = new Promise(resolve => {
onClose = resolve;
});
const Wrapper = () => {
const [search, setSearch] = useState(searchRef);
const containerRef = useRef();
searchRef = search;
const handleSearch = value => {
setSearch(value);
};
const handleClose = () => {
onClose();
};
const container = () => containerRef.current;
return (
<div ref={containerRef}>
<button id="exit">exit</button>
<SelectPicker
container={container}
search={search}
defaultOpen
onClose={handleClose}
onSearch={handleSearch}
data={data}
/>
</div>
);
};
Wrapper.displayName = 'WrapperSelectPicker';
const { container } = render(<Wrapper />);

const exit = container?.querySelector('#exit');
const input = container.querySelector('.rs-picker-search-bar-input');

// change search
fireEvent.change(input, { target: { value: 'a' } });
assert.equal(searchRef, 'a');

// close select
fireEvent.mouseDown(exit, { bubbles: true });

promise.then(() => {
assert.equal(searchRef, '');
done();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to existing test scripts and see how to use sinon.spy() to validate a callback has been called with correct arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified it, but I think this test code does not reflect its operation scenario, How do you think

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean the way this is written should not be suitable for such a description, should is "shoule call onSearch when closed", or other

Copy link
Member

Choose a reason for hiding this comment

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

I mean the way this is written should not be suitable for such a description, should is "shoule call onSearch when closed", or other

The description was not quite suitable in the first place. "Should call onSearch with '' when closed" is exactly what should be tested here. The goal of unit tests is to make sure the APIs we provide (onSearch) work as expected (called with '' when closed), rather than to cover how users are using our APIs (e.g. updating a state when onSearch is called).

});