feat(Autocomplete): add controllable isOpen, inputValue props; extend text behaviour on item select #2567

maxcheremisin · 2023-08-25T14:07:41Z

Purpose of PR

What's changed:

❌ clearAfterSelect (boolean) replaced with 🆕 textOnAfterSelect ('clear' | 'preserve' | 'replace') to be able to preserve the text input value when item selected
🆕 inputValue prop added to be able to clear the text input programmatically (e.g. on menu close)
🆕 isOpen prop added (and corresponding onOpen, onClose handlers) to be able to control menu state and subscribe to its change

Screen.Recording.2023-08-25.at.16.05.03.mov

vercel · 2023-08-25T14:07:46Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
forma-36	✅ Ready (Inspect)	Visit Preview	Aug 29, 2023 9:15pm

changeset-bot · 2023-08-25T14:10:06Z

🦋 Changeset detected

Latest commit: 13e66bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages

Name	Type
@contentful/f36-autocomplete	Minor
@contentful/f36-accordion	Minor
@contentful/f36-asset	Minor
@contentful/f36-badge	Minor
@contentful/f36-button	Minor
@contentful/f36-card	Minor
@contentful/f36-collapse	Minor
@contentful/f36-copybutton	Minor
@contentful/f36-core	Minor
@contentful/f36-datetime	Minor
@contentful/f36-datepicker	Minor
@contentful/f36-drag-handle	Minor
@contentful/f36-entity-list	Minor
@contentful/f36-empty-state	Minor
@contentful/f36-forms	Minor
@contentful/f36-icon	Minor
@contentful/f36-list	Minor
@contentful/f36-menu	Minor
@contentful/f36-modal	Minor
@contentful/f36-note	Minor
@contentful/f36-notification	Minor
@contentful/f36-pagination	Minor
@contentful/f36-pill	Minor
@contentful/f36-popover	Minor
@contentful/f36-skeleton	Minor
@contentful/f36-spinner	Minor
@contentful/f36-table	Minor
@contentful/f36-tabs	Minor
@contentful/f36-text-link	Minor
@contentful/f36-tooltip	Minor
@contentful/f36-typography	Minor
@contentful/f36-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

maxcheremisin · 2023-08-25T14:10:58Z

packages/components/autocomplete/src/Autocomplete.tsx

+              }}
+              onBlur={(e) => {
+                onBlur?.(e as React.FocusEvent<HTMLInputElement>);
+                inputProps.onBlur(e);


was missing and didn't fire onBlur properly

maxcheremisin · 2023-08-25T14:12:23Z

packages/components/autocomplete/src/Autocomplete.tsx

-          if (!closeAfterSelect) {
-            toggleMenu();
-          }


this logic is moved to stateReducer so it doesnt cause extra rerenders and doesnt fire onIsOpenChange

github-actions · 2023-08-25T14:15:47Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
CommonJS	129.48 KB (+0.15% 🔺)	2.6 s (+0.15% 🔺)	466 ms (+10.33% 🔺)	3.1 s
Module	125.96 KB (+0.16% 🔺)	2.6 s (+0.16% 🔺)	308 ms (+4.75% 🔺)	2.9 s

maxcheremisin · 2023-08-25T14:19:47Z

packages/components/autocomplete/src/Autocomplete.tsx

-          if (clearAfterSelect) {
-            handleInputValueChange('');
-          }


moved to onInputValueChange

Chaoste

Looks good to me - just a nitpick on the testing style. I didn't approve as this should be done by the owning team.

Chaoste · 2023-08-28T06:34:32Z

packages/components/autocomplete/src/Autocomplete.test.tsx

+      const input = screen.getByTestId('cf-autocomplete-input');
+      const container = screen.getByTestId('cf-autocomplete-container');
+
+      // Type one letter in the input to open the list
+      fireEvent.input(input, {
+        target: {
+          value: 'a',
+        },
+      });
+
+      // checks if the list is visible
+      await waitFor(() => {
+        expect(container).toBeVisible();
+      });
+
+      // go to the list first item
+      fireEvent.keyDown(input, {
+        key: 'ArrowDown',
+      });
+
+      // press Enter to select the item
+      fireEvent.keyDown(input, {
+        key: 'Enter',
+      });


[nitpick]

Considering that we try to replace usage of fireEvent with userEvent for more realistic input testing, as well as some other recommended testing styles (e.g. avoid test IDs but use realistic queries), I would rewrite it like this:

Suggested change

const input = screen.getByTestId('cf-autocomplete-input');

const container = screen.getByTestId('cf-autocomplete-container');

// Type one letter in the input to open the list

fireEvent.input(input, {

target: {

value: 'a',

},

});

// checks if the list is visible

await waitFor(() => {

expect(container).toBeVisible();

});

// go to the list first item

fireEvent.keyDown(input, {

key: 'ArrowDown',

});

// press Enter to select the item

fireEvent.keyDown(input, {

key: 'Enter',

});

await userEvent.type(screen.getByRole('textbox'), 'a');

expect(await screen.findByRole('menuitem', { name: /apple/ }).toBeInTheDocument();

await userEvent.type(screen.getByRole('textbox'), '{arrowdown}{enter}');

All of these changes are also further explained in this blog post which I can highly recommend to understand why we shouldn't use test IDs, etc.

i just copied one of the test cases and adjusted it for the new prop. but i refactored the test file now using user event and getByRole

Thank you! I know that many of our repositories are still in this old state. Just want to avoid that reproduce it - maybe we can get rid of these things by slowly refactoring it bit by bit.

denkristoffer

Prop changes like this are a breaking change. Since the Autocomplete is considered stable we will have to deprecate instead and remove in v5.

maxcheremisin · 2023-08-29T10:09:14Z

Prop changes like this are a breaking change. Since the Autocomplete is considered stable we will have to deprecate instead and remove in v5.

thanks! done

…aviour on item select

cf-remylenoir

It looks good, could you please add an example of using the new props in the documentation and storybook as per in the video? I was not able to reproduce it myself 🤔

maxcheremisin · 2023-08-29T16:57:40Z

It looks good, could you please add an example of using the new props in the documentation and storybook as per in the video? I was not able to reproduce it myself 🤔

curious what you think - there is an example for textOnAfterSelect, but i didn't add more examples of inputValue and isOpen to the docs because the use case is pretty niche and it might overload the docs with too much information.

but i defo can add more stories to the storybook

maxcheremisin · 2023-08-29T20:39:18Z

It looks good, could you please add an example of using the new props in the documentation and storybook as per in the video? I was not able to reproduce it myself 🤔

curious what you think - there is an example for textOnAfterSelect, but i didn't add more examples of inputValue and isOpen to the docs because the use case is pretty niche and it might overload the docs with too much information.

but i defo can add more stories to the storybook

extended ControlledFromOutside story with more props

cf-remylenoir

Looking good 👍 I noted down the deprecation for the next major release.

maxcheremisin requested review from Chaoste and cf-remylenoir August 25, 2023 14:07

maxcheremisin requested review from Lelith and a team as code owners August 25, 2023 14:07

maxcheremisin requested review from mshaaban0 and giotiskl August 25, 2023 14:07

maxcheremisin force-pushed the feat/Autocomplete-isOpen|textOnAfterSelect branch from 8036755 to d1b88c8 Compare August 25, 2023 14:10

maxcheremisin commented Aug 25, 2023

View reviewed changes

vercel bot deployed to Preview August 25, 2023 14:13 View deployment

maxcheremisin commented Aug 25, 2023

View reviewed changes

Chaoste reviewed Aug 28, 2023

View reviewed changes

vercel bot deployed to Preview August 28, 2023 12:35 View deployment

denkristoffer requested changes Aug 29, 2023

View reviewed changes

maxcheremisin force-pushed the feat/Autocomplete-isOpen|textOnAfterSelect branch from 6f7745d to 75624e3 Compare August 29, 2023 10:06

maxcheremisin requested a review from denkristoffer August 29, 2023 10:09

vercel bot deployed to Preview August 29, 2023 10:09 View deployment

maxcheremisin force-pushed the feat/Autocomplete-isOpen|textOnAfterSelect branch from 75624e3 to cddd58d Compare August 29, 2023 10:10

vercel bot deployed to Preview August 29, 2023 10:13 View deployment

maxcheremisin added 3 commits August 29, 2023 17:15

feat(Autocomplete): add controllable inputValue prop; extend text beh…

d959bba

…aviour on item select

refactor(Autocomplete): test

d5e942e

feat(Autocomplete): return clearAfterSelect and deprecate it

f40922f

maxcheremisin force-pushed the feat/Autocomplete-isOpen|textOnAfterSelect branch from cddd58d to f40922f Compare August 29, 2023 15:15

vercel bot deployed to Preview August 29, 2023 15:19 View deployment

cf-remylenoir reviewed Aug 29, 2023

View reviewed changes

maxcheremisin added 2 commits August 29, 2023 22:34

fix: useCombobox internally changes input value on blur

44f522a

feat: extend ControlledFromOutside with new props

13e66bb

maxcheremisin requested a review from cf-remylenoir August 29, 2023 20:39

vercel bot deployed to Preview August 29, 2023 21:15 View deployment

cf-remylenoir approved these changes Aug 30, 2023

View reviewed changes

denkristoffer approved these changes Aug 30, 2023

View reviewed changes

maxcheremisin merged commit 51eb9f4 into main Aug 30, 2023
3 checks passed

maxcheremisin deleted the feat/Autocomplete-isOpen|textOnAfterSelect branch August 30, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Autocomplete): add controllable isOpen, inputValue props; extend text behaviour on item select #2567

feat(Autocomplete): add controllable isOpen, inputValue props; extend text behaviour on item select #2567

maxcheremisin commented Aug 25, 2023 •

edited

Loading

vercel bot commented Aug 25, 2023 •

edited

Loading

changeset-bot bot commented Aug 25, 2023 •

edited

Loading

maxcheremisin Aug 25, 2023

maxcheremisin Aug 25, 2023

github-actions bot commented Aug 25, 2023 •

edited

Loading

maxcheremisin Aug 25, 2023

Chaoste left a comment •

edited

Loading

Chaoste Aug 28, 2023

maxcheremisin Aug 28, 2023

Chaoste Aug 28, 2023

denkristoffer left a comment

maxcheremisin commented Aug 29, 2023

cf-remylenoir left a comment

maxcheremisin commented Aug 29, 2023

maxcheremisin commented Aug 29, 2023

cf-remylenoir left a comment

feat(Autocomplete): add controllable isOpen, inputValue props; extend text behaviour on item select #2567

feat(Autocomplete): add controllable isOpen, inputValue props; extend text behaviour on item select #2567

Conversation

maxcheremisin commented Aug 25, 2023 • edited Loading

Purpose of PR

vercel bot commented Aug 25, 2023 • edited Loading

changeset-bot bot commented Aug 25, 2023 • edited Loading

🦋 Changeset detected

maxcheremisin Aug 25, 2023

Choose a reason for hiding this comment

maxcheremisin Aug 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2023 • edited Loading

size-limit report 📦

maxcheremisin Aug 25, 2023

Choose a reason for hiding this comment

Chaoste left a comment • edited Loading

Choose a reason for hiding this comment

Chaoste Aug 28, 2023

Choose a reason for hiding this comment

maxcheremisin Aug 28, 2023

Choose a reason for hiding this comment

Chaoste Aug 28, 2023

Choose a reason for hiding this comment

denkristoffer left a comment

Choose a reason for hiding this comment

maxcheremisin commented Aug 29, 2023

cf-remylenoir left a comment

Choose a reason for hiding this comment

maxcheremisin commented Aug 29, 2023

maxcheremisin commented Aug 29, 2023

cf-remylenoir left a comment

Choose a reason for hiding this comment

maxcheremisin commented Aug 25, 2023 •

edited

Loading

vercel bot commented Aug 25, 2023 •

edited

Loading

changeset-bot bot commented Aug 25, 2023 •

edited

Loading

github-actions bot commented Aug 25, 2023 •

edited

Loading

Chaoste left a comment •

edited

Loading