Skip to content

Conversation

apliano
Copy link
Contributor

@apliano apliano commented Feb 6, 2024

WHY are these changes introduced?

When a Combobox has children that is rendered based on an input change, our previous implementation failed to mark the popover as active. This was due to the fact that the handleChange was triggered before the re render of the parent component, and at that point the Combobox is unaware of the "future" children.

As a small example:

    function ComboboxWithHandledChange() {
      const handleChange = (value: string) => {
        setInputValue(value);
      };

      const [inputValue, setInputValue] = useState('');

      return (
        <Combobox
          activator={
            <Combobox.TextField
              onChange={handleChange}
              label=""
              value=""
              autoComplete="off"
            />
          }
        >
          {inputValue.length > 0 ? (
            <Listbox>
              <Listbox.Option accessibilityLabel="Option 1" value="option1" />
            </Listbox>
          ) : null}
        </Combobox>
      );
    }
  1. Initially the input is empty, hence the Combobox has no children
  2. The user types something, let's say "t"
  3. The callback handleChange of the Combobox is called, which checks the value shouldOpen -> Boolean(!popoverActive && Children.count(children) > 0). At this point the parent component has not been re-rendered, so the children is still null and not a Listbox; this results on popoverActive not being set to true.
  4. The popover is then not marked as active.
  5. After the parent re-renders, the Combobox re-renders again but as popoverActive is set to false, the Popover is not marked as active.

With the fix, we have changed shouldOpen to assert if there's been a callback call (handleChange or handleFocus) and then in rendering time, we set the active property of the Popover based on shouldOpen has been set and if there's any Children.

WHAT is this pull request doing?

This PR addresses the aforementioned issue, changing the logic of shouldOpen to just ensure if the popover is currently active or not. Then when rendering the Popover we ensure that shouldOpen has been called (which has set popoverActive to true) and that there's children passed to the Combobox.

How to 🎩

  • Check the new Unit Test added
  • Run Storybook and with this path: ?path=/story/all-components-combobox--with-children-based-on-input

🎩 checklist

@apliano apliano force-pushed the ap/combobox-input-fix branch 2 times, most recently from fa8800d to fc8a4ee Compare February 6, 2024 15:15
@apliano apliano changed the title Fix Combobox HandleChange [Combobox] - Fix handleChange when children changes based on input Feb 6, 2024
@apliano apliano force-pushed the ap/combobox-input-fix branch 2 times, most recently from 3b1e5bf to f5450ec Compare February 6, 2024 15:40
@apliano apliano self-assigned this Feb 6, 2024
@apliano apliano requested review from chloerice and voiid February 6, 2024 15:42
@apliano apliano marked this pull request as ready for review February 6, 2024 15:42
@apliano apliano force-pushed the ap/combobox-input-fix branch from f5450ec to a4617e0 Compare February 6, 2024 15:59
@apliano apliano force-pushed the ap/combobox-input-fix branch from a4617e0 to 87eaba1 Compare February 6, 2024 16:05
@apliano apliano marked this pull request as draft February 6, 2024 16:05
@apliano apliano marked this pull request as ready for review February 6, 2024 16:25
Copy link

@romidane romidane left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @apliano! :shipit:

Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
@apliano apliano merged commit 0b38b61 into main Feb 9, 2024
@apliano apliano deleted the ap/combobox-input-fix branch February 9, 2024 12:57
sam-b-rose pushed a commit that referenced this pull request Feb 15, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris-icons@8.3.0

### Minor Changes

- [#11526](#11526)
[`b65f1e679`](b65f1e6)
Thanks [@j-wanita](https://github.com/j-wanita)! - Add list icons for
product, collection, metaobject and text


- [#11531](#11531)
[`78ed5fe0d`](78ed5fe)
Thanks [@j-wanita](https://github.com/j-wanita)! - Updated metaobject,
metaobject reference, and metaobject filled icons

## @shopify/polaris-migrator@0.28.0

### Minor Changes

- [#11596](#11596)
[`c8fabc011`](c8fabc0)
Thanks [@lgriffee](https://github.com/lgriffee)! - Created migration to
replace deprecated `font` custom properties in polaris-react v13.0.0

### Patch Changes

- [#11603](#11603)
[`2c53d6476`](2c53d64)
Thanks [@lgriffee](https://github.com/lgriffee)! - Updated migration to
replace deprecated `font` custom properties in polaris-react v13.0.0

## @shopify/polaris@12.16.0

### Minor Changes

- [#11585](#11585)
[`1ba3181b6`](1ba3181)
Thanks [@tauthomas01](https://github.com/tauthomas01)! - Added a
`disabled` prop to `ResourceItem`


- [#11568](#11568)
[`525194767`](5251947)
Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the stacking
logic of multiple Toasts to take up less screen real estate


- [#11587](#11587)
[`5ab254b3b`](5ab254b)
Thanks [@sainihas](https://github.com/sainihas)! - Update dropzone
container background color when no Outline

### Patch Changes

- [#11581](#11581)
[`47dac1b2e`](47dac1b)
Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed an issue
where scrollbars weren't showing up in IndexTable on mac os when show
when scrolling preference is selected


- [#11560](#11560)
[`0b38b6115`](0b38b61)
Thanks [@apliano](https://github.com/apliano)! - Fixed `Combobox` not
rendering `Popover` until the second firing of the `onChange` event


- [#11584](#11584)
[`23d8297ff`](23d8297)
Thanks [@mrcthms](https://github.com/mrcthms)! - Updated
`useIsSelectAllActionsSticky` logic to not set any sticky behaviour if
we do not have access to the root element


- [#11543](#11543)
[`165bc6eae`](165bc6e)
Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed `IndexFilters`
height changing when toggling between default and filtering modes


- [#11563](#11563)
[`3937739d2`](3937739)
Thanks [@chloerice](https://github.com/chloerice)! - Fixed
`FormLayout.Item` overflowing viewport at xs breakpoint when user
settings enlarge text size


- [#11595](#11595)
[`f829ed487`](f829ed4)
Thanks [@oksanashopify](https://github.com/oksanashopify)! - Updated
DropZone minimum size from 50px to 40px to fit within a small Thumbnail

- Updated dependencies
\[[`b65f1e679`](b65f1e6),
[`78ed5fe0d`](78ed5fe)]:
    -   @shopify/polaris-icons@8.3.0

## polaris.shopify.com@0.63.0

### Minor Changes

- [#11596](#11596)
[`c8fabc011`](c8fabc0)
Thanks [@lgriffee](https://github.com/lgriffee)! - Created migration to
replace deprecated `font` custom properties in polaris-react v13.0.0


- [#11568](#11568)
[`525194767`](5251947)
Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the stacking
logic of multiple Toasts to take up less screen real estate

### Patch Changes

- Updated dependencies
\[[`b65f1e679`](b65f1e6),
[`47dac1b2e`](47dac1b),
[`0b38b6115`](0b38b61),
[`23d8297ff`](23d8297),
[`1ba3181b6`](1ba3181),
[`165bc6eae`](165bc6e),
[`78ed5fe0d`](78ed5fe),
[`3937739d2`](3937739),
[`f829ed487`](f829ed4),
[`525194767`](5251947),
[`5ab254b3b`](5ab254b)]:
    -   @shopify/polaris-icons@8.3.0
    -   @shopify/polaris@12.16.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
…hopify#11560)

<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Open it as a draft if it’s a work in progress
-->

### WHY are these changes introduced?

When a Combobox has children that is rendered based on an input change,
our previous implementation failed to mark the popover as active. This
was due to the fact that the `handleChange` was triggered before the re
render of the parent component, and at that point the `Combobox` is
unaware of the "future" children.

As a small example: 

```
    function ComboboxWithHandledChange() {
      const handleChange = (value: string) => {
        setInputValue(value);
      };

      const [inputValue, setInputValue] = useState('');

      return (
        <Combobox
          activator={
            <Combobox.TextField
              onChange={handleChange}
              label=""
              value=""
              autoComplete="off"
            />
          }
        >
          {inputValue.length > 0 ? (
            <Listbox>
              <Listbox.Option accessibilityLabel="Option 1" value="option1" />
            </Listbox>
          ) : null}
        </Combobox>
      );
    }
```

1. Initially the input is empty, hence the `Combobox` has no children
2. The user types something, let's say "t"
3. The callback `handleChange` of the `Combobox` is called, which checks
the value `shouldOpen` -> `Boolean(!popoverActive &&
Children.count(children) > 0)`. At this point the parent component has
not been re-rendered, so the children is still `null` and not a
`Listbox`; this results on `popoverActive` not being set to true.
4. The popover is then not marked as `active`.
5. After the parent re-renders, the `Combobox` re-renders again but as
`popoverActive` is set to false, the `Popover` is not marked as
`active`.

With the fix, we have changed `shouldOpen` to assert if there's been a
callback call (`handleChange` or `handleFocus`) and then in rendering
time, we set the `active` property of the `Popover` based on
`shouldOpen` has been set and if there's any Children.

### WHAT is this pull request doing?

This PR addresses the aforementioned issue, changing the logic of
`shouldOpen` to just ensure if the popover is currently active or not.
Then when rendering the `Popover` we ensure that `shouldOpen` has been
called (which has set `popoverActive` to true) and that there's children
passed to the `Combobox`.

### How to 🎩

- Check the new Unit Test added
- Run Storybook and with this path:
`?path=/story/all-components-combobox--with-children-based-on-input`

### 🎩 checklist

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [x] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [x] Updated the component's `README.md` with documentation changes
- [x] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide

---------

Co-authored-by: Chloe Rice <chloerice@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants