fix: fix selectPicker when search is controlled and exit Dropdown wit… #2411

myNameIsDu · 2022-03-22T06:44:35Z

fix selectPicker when search is controlled and exit Dropdown without resetting external search

…hout resetting external search

vercel · 2022-03-22T06:44:40Z

@myNameIsDu is attempting to deploy a commit to the rsuite Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2022-03-22T06:44:41Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

rsuite-nextjs – ./docs

🔍 Inspect: https://vercel.com/rsuite/rsuite-nextjs/B3pvCDUNtue8MQm1XW5UQnihDdrK
✅ Preview: https://rsuite-nextjs-git-fork-mynameisdu-fix-v5-selectpicker-rsuite.vercel.app

rsuite-v4 – ./docs

🔍 Inspect: https://vercel.com/rsuite/rsuite-v4/CPUtsZ5qGpDXvcMSbcoSRFJgKcF7
✅ Preview: Canceled

[Deployment for b3793f9 canceled]

codesandbox-ci · 2022-03-22T06:46:14Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d7653ee:

Sandbox	Source
rsuite-tp-ci	Configuration

codecov · 2022-03-22T06:48:13Z

Codecov Report

Merging #2411 (d7653ee) into main (ab4c721) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2411      +/-   ##
==========================================
+ Coverage   85.54%   85.59%   +0.04%     
==========================================
  Files         274      275       +1     
  Lines        8648     8655       +7     
  Branches     2428     2433       +5     
==========================================
+ Hits         7398     7408      +10     
+ Misses        660      658       -2     
+ Partials      590      589       -1

Flag	Coverage Δ
ChromeCi	`85.59% <100.00%> (+0.04%)`	⬆️
Firefox	`85.58% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/SelectPicker/SelectPicker.tsx	`90.09% <100.00%> (+0.09%)`	⬆️
src/Overlay/OverlayTrigger.tsx	`62.69% <0.00%> (-0.80%)`	⬇️
src/Dropdown/DropdownMenu.tsx	`92.68% <0.00%> (-0.50%)`	⬇️
src/InputPicker/InputPicker.tsx	`86.51% <0.00%> (ø)`
src/Form/useFormClassNames.ts	`100.00% <0.00%> (ø)`
src/Picker/DropdownMenuCheckItem.tsx	`95.23% <0.00%> (+0.23%)`	⬆️
src/Toggle/Toggle.tsx	`92.59% <0.00%> (+0.28%)`	⬆️
src/Checkbox/Checkbox.tsx	`92.68% <0.00%> (+0.57%)`	⬆️
src/Form/Form.tsx	`87.27% <0.00%> (+0.66%)`	⬆️
src/Menu/MenuItem.tsx	`81.08% <0.00%> (+3.30%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab4c721...d7653ee. Read the comment docs.

simonguo · 2022-03-29T07:29:29Z

src/SelectPicker/SelectPicker.tsx

@@ -288,6 +288,7 @@ const SelectPicker = React.forwardRef(
    const handleExited = useCallback(() => {
      setSearchKeyword('');
      setActive(false);
+      onSearch?.('');
      onClose?.();
    }, [onClose, setSearchKeyword]);


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

Yeah,I added it.

SevenOutman · 2022-03-24T06:46:41Z

src/SelectPicker/test/SelectPickerSpec.js

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


cleanup() is unnecessary here.

Yeah,I removed it

SevenOutman · 2022-04-03T11:52:10Z

src/SelectPicker/test/SelectPickerSpec.js

+  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();
+    });
+  });


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

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

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

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

SevenOutman · 2022-04-03T19:34:37Z

src/SelectPicker/test/SelectPickerSpec.js

+    await waitForElementToBeRemoved(document.querySelector('.rs-picker-search-bar-input'));
+
+    assert.isTrue(handleSearch.calledOnce);


Avoid mentioning implementation detail in test cases. Simply use waitFor from @testing-library/react for async assertions.

Arguments of onSearch call should also be asserted.

SevenOutman · 2022-04-03T19:40:09Z

src/SelectPicker/test/SelectPickerSpec.js

+    const Wrapper = () => {
+      return (
+        <>
+          <button id="exit">exit</button>
+          <SelectPicker container={container} defaultOpen onSearch={handleSearch} data={data} />
+        </>
+      );
+    };
+    Wrapper.displayName = 'WrapperSelectPicker';
+    let { container } = render(<Wrapper />);


Unnecessary wrapping. Simply render the fragment.

SevenOutman · 2022-04-03T19:40:33Z

src/SelectPicker/test/SelectPickerSpec.js

+    };
+    Wrapper.displayName = 'WrapperSelectPicker';
+    let { container } = render(<Wrapper />);
+    const exit = container.querySelector('#exit');


Use getElementById when possible.

The container doesn't have the getElementById method

document should be available in test scripts.

SevenOutman · 2022-04-04T06:30:00Z

src/SelectPicker/test/SelectPickerSpec.js

+    await waitFor(() => assert.isTrue(handleClose.calledOnce));
+
+    assert.isTrue(handleSearch.calledOnce);


Duplicated assertions. You could wrap multiple assertions within waitFor.

Great writing style

fix: fix selectPicker when search is controlled and exit Dropdown wit…

ee681c6

…hout resetting external search

vercel bot deployed to Preview – rsuite-nextjs March 22, 2022 06:44 View deployment

myNameIsDu requested a review from superman66 March 22, 2022 06:45

vercel bot temporarily deployed to Preview – rsuite-v4 March 23, 2022 01:26 Inactive

vercel bot temporarily deployed to Preview – rsuite-v4 March 24, 2022 07:00 Inactive

simonguo requested changes Mar 29, 2022

View reviewed changes

fix: selectPicker handleExited add onSearch dependency

b686373

vercel bot deployed to Preview – rsuite-nextjs March 31, 2022 03:18 View deployment

myNameIsDu requested a review from simonguo March 31, 2022 03:31

vercel bot temporarily deployed to Preview – rsuite-v4 April 2, 2022 02:32 Inactive

simonguo approved these changes Apr 2, 2022

View reviewed changes

SevenOutman requested changes Apr 3, 2022

View reviewed changes

perf: remove unnecessary cleanup

755930e

vercel bot deployed to Preview – rsuite-nextjs April 3, 2022 15:05 View deployment

vercel bot deployed to Preview – rsuite-nextjs April 3, 2022 16:28 View deployment

perf: optimized use of sinon.spy()

61cd20f

myNameIsDu force-pushed the fix/v5-selectPicker branch from bfd9b2d to 61cd20f Compare April 3, 2022 16:36

vercel bot deployed to Preview – rsuite-nextjs April 3, 2022 16:37 View deployment

SevenOutman requested changes Apr 3, 2022

View reviewed changes

vercel bot deployed to Preview – rsuite-nextjs April 4, 2022 05:08 View deployment

fix: modify test case description

99a2b58

myNameIsDu force-pushed the fix/v5-selectPicker branch from 8d48674 to 064deca Compare April 4, 2022 05:42

vercel bot deployed to Preview – rsuite-nextjs April 4, 2022 05:42 View deployment

myNameIsDu force-pushed the fix/v5-selectPicker branch from 064deca to b4e78d7 Compare April 4, 2022 05:44

vercel bot deployed to Preview – rsuite-nextjs April 4, 2022 05:47 View deployment

perf: optimize the code

b3793f9

myNameIsDu force-pushed the fix/v5-selectPicker branch from b4e78d7 to b3793f9 Compare April 4, 2022 05:53

vercel bot deployed to Preview – rsuite-nextjs April 4, 2022 05:53 View deployment

SevenOutman requested changes Apr 4, 2022

View reviewed changes

vercel bot temporarily deployed to Preview – rsuite-v4 April 5, 2022 08:20 Inactive

perf: optimize the code

d7653ee

vercel bot deployed to Preview – rsuite-nextjs April 6, 2022 17:31 View deployment

SevenOutman merged commit c6e5d54 into rsuite:main Apr 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix selectPicker when search is controlled and exit Dropdown wit… #2411

fix: fix selectPicker when search is controlled and exit Dropdown wit… #2411

myNameIsDu commented Mar 22, 2022

vercel bot commented Mar 22, 2022

vercel bot commented Mar 22, 2022 •

edited

codesandbox-ci bot commented Mar 22, 2022 •

edited

codecov bot commented Mar 22, 2022 •

edited

simonguo Mar 29, 2022

myNameIsDu Mar 31, 2022

SevenOutman Mar 24, 2022

myNameIsDu Apr 3, 2022

SevenOutman Apr 3, 2022

myNameIsDu Apr 3, 2022

myNameIsDu Apr 3, 2022

SevenOutman Apr 3, 2022

SevenOutman Apr 3, 2022

myNameIsDu Apr 4, 2022

SevenOutman Apr 3, 2022

myNameIsDu Apr 4, 2022

SevenOutman Apr 3, 2022

myNameIsDu Apr 4, 2022

SevenOutman Apr 4, 2022

SevenOutman Apr 4, 2022

myNameIsDu Apr 6, 2022

		await waitForElementToBeRemoved(document.querySelector('.rs-picker-search-bar-input'));

		assert.isTrue(handleSearch.calledOnce);

		await waitFor(() => assert.isTrue(handleClose.calledOnce));

		assert.isTrue(handleSearch.calledOnce);

fix: fix selectPicker when search is controlled and exit Dropdown wit… #2411

fix: fix selectPicker when search is controlled and exit Dropdown wit… #2411

Conversation

myNameIsDu commented Mar 22, 2022

vercel bot commented Mar 22, 2022

vercel bot commented Mar 22, 2022 • edited

rsuite-nextjs – ./docs

rsuite-v4 – ./docs

codesandbox-ci bot commented Mar 22, 2022 • edited

codecov bot commented Mar 22, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Mar 22, 2022 •

edited

codesandbox-ci bot commented Mar 22, 2022 •

edited

codecov bot commented Mar 22, 2022 •

edited