Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

ShabanaRumane
Copy link
Contributor

@ShabanaRumane ShabanaRumane commented Jan 31, 2024

closes: #11534

WHY are these changes introduced?

The combobox does not have a prop to accept maxHeight to set the height from default. The height prop on combobox when set, gives empty space when filtering the suggestions.
31-31-aqwrw-uillp

This PR introduces the height component to the Popover.pane. The fix was for TagAutocomplete in web. I have not been able to figure out what the issue was and how to test this change against them.

WHAT is this pull request doing?

This PR adds the minHeight and maxHeight as props for combobox and popover.pane component.

video of after changes
Screen.Recording.2024-01-31.at.11.01.53.AM.mov

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Using main branch:

  • For MultiselectVerticalContent update the list to add more options, say 30-40 items more.
  • Set height in the combobox to say 300px to limit the height of the suggestions dropdown
  • Filter suggestions and observe the extra space at the bottom of the filtered list.

Using this branch:

  • For MultiselectVerticalContent update the list to add more options, say 30-40 items more.
  • Set maxHeight in the combobox to 300px
  • Filter suggestions and observe that there are no extra space at the bottom of the filtered list.

🎩 checklist

@ShabanaRumane ShabanaRumane force-pushed the add-max-height-to-popover-pane branch 3 times, most recently from 8930bfd to 46fdcac Compare February 5, 2024 14:54
@ShabanaRumane ShabanaRumane marked this pull request as ready for review February 5, 2024 18:56
? {height, maxHeight: height, minHeight: height}
: undefined;

const style = {

Choose a reason for hiding this comment

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

I like that this is more predictable now! It's just like one would expect it to behave if it where just CSS.

@chloerice
Copy link
Member

Hey @ShabanaRumane 👋🏽 TagsAutocomplete was given a fixed height intentionally to prevent jank when searching. The height prop on Combobox is meant to support you providing the popover a fixed height, that's why min and max are also set. If you don't want the Popover to have a fixed height, you can you just remove the height prop from the Combobox in TagsAutocomplete.

@ShabanaRumane
Copy link
Contributor Author

Hey @ShabanaRumane 👋🏽 TagsAutocomplete was given a fixed height intentionally to prevent jank when searching. The height prop on Combobox is meant to support you providing the popover a fixed height, that's why min and max are also set. If you don't want the Popover to have a fixed height, you can you just remove the height prop from the Combobox in TagsAutocomplete.

@chloerice - We do need the height. When the height prop is set in the combobox, the height of the popover is fixed. It does not collapse if the search result is less than the height.
In the polaris example, if we enter text for search, the popover size is reduced based on the results. But when the height prop is provided, it does not reduce. It stays fixed. I am trying to fix this problem with this PR.

@chloerice
Copy link
Member

Hey @ShabanaRumane 👋🏽 TagsAutocomplete was given a fixed height intentionally to prevent jank when searching. The height prop on Combobox is meant to support you providing the popover a fixed height, that's why min and max are also set. If you don't want the Popover to have a fixed height, you can you just remove the height prop from the Combobox in TagsAutocomplete.

@chloerice - We do need the height. When the height prop is set in the combobox, the height of the popover is fixed. It does not collapse if the search result is less than the height. In the polaris example, if we enter text for search, the popover size is reduced based on the results. But when the height prop is provided, it does not reduce. It stays fixed. I am trying to fix this problem with this PR.

Hey @ShabanaRumane! It sounds like what you want is more fine-grained control so that you can change the TagsAutocomplete from having a fixed height to having a min-height only? If you want the combobox to respond to the height of the contents, removing the height will give you that behavior. While that is not the intended UX, I understand that there are use cases where a Popover needs min/max only and don't have any issues with opening up the API to give that full control to consumers 💯

'@shopify/polaris': minor
---

Adding maxHeight and minHeight as props to Popover.Pane and Combobox
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adding maxHeight and minHeight as props to Popover.Pane and Combobox
Added support for setting `maxHeight` and `minHeight` on `Popover.Pane` and `Combobox`

@aaronccasanova aaronccasanova force-pushed the add-max-height-to-popover-pane branch from 46fdcac to 439e524 Compare April 19, 2024 21:52
@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@translation-platform
Copy link
Contributor

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

  • The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

@ShabanaRumane ShabanaRumane force-pushed the add-max-height-to-popover-pane branch from 439e524 to 55560cd Compare April 22, 2024 13:49
@github-actions github-actions bot removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 22, 2024
@ShabanaRumane ShabanaRumane requested review from kyledurand, laurkim and sophschneider and removed request for kyledurand April 22, 2024 13:49
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Makes sense to me, just one small clean up note 👍

@ShabanaRumane ShabanaRumane force-pushed the add-max-height-to-popover-pane branch from 55560cd to 47d1c8b Compare April 23, 2024 15:27
@ShabanaRumane ShabanaRumane merged commit bcd16df into main Apr 23, 2024
@ShabanaRumane ShabanaRumane deleted the add-max-height-to-popover-pane branch April 23, 2024 19:26
sophschneider pushed a commit that referenced this pull request Apr 24, 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@13.2.0

### Minor Changes

- [#11535](#11535)
[`bcd16df24`](bcd16df)
Thanks [@ShabanaRumane](https://github.com/ShabanaRumane)! - Added
support for setting `maxHeight` and `minHeight` on `Popover.Pane` and
`Combobox`


- [#11907](#11907)
[`45308c97a`](45308c9)
Thanks [@zakwarsame](https://github.com/zakwarsame)! - Added an optional
`fiterLabel` prop to `ActionList` to allow for a custom placeholder

### Patch Changes

- [#11897](#11897)
[`a83084b3b`](a83084b)
Thanks [@jesstelford](https://github.com/jesstelford)! - Fixed edges of
disabled `IndexTable.Row` `Checkbox` triggering selection


- [#11924](#11924)
[`5ec70e688`](5ec70e6)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to jest
29


- [#11929](#11929)
[`9ee700be6`](9ee700b)
Thanks [@sophschneider](https://github.com/sophschneider)! - Rounded
`Navigation` at `mdDown` behind a feature flag


- [#11923](#11923)
[`ce13c4366`](ce13c43)
Thanks [@jesstelford](https://github.com/jesstelford)! - Update dev
dependency: `postcss-import@^15.1.0` -> `postcss-import@^16.1.0`


- [#11925](#11925)
[`364ada59e`](364ada5)
Thanks [@sophschneider](https://github.com/sophschneider)! - Updated
Frame to only apply rounded Frame when passed a `topBar`


- [#11734](#11734)
[`1fef06256`](1fef062)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to
Storybook v8


- [#11898](#11898)
[`1539f0e7c`](1539f0e)
Thanks [@jesstelford](https://github.com/jesstelford)! - Removed extra
padding around `IndexTable.Row` `Checkbox`


- [#11927](#11927)
[`5a32a3ff6`](5a32a3f)
Thanks [@sophschneider](https://github.com/sophschneider)! - Added
`prefers-reduced-motion` media queries to `Frame` width transitions


- [#11930](#11930)
[`b111629d7`](b111629)
Thanks [@jesstelford](https://github.com/jesstelford)! - Migrate
Storybook stories to CSF v3


- [#11805](#11805)
[`0a9b72721`](0a9b727)
Thanks [@LA1CH3](https://github.com/LA1CH3)! - Fixed `IndexTable`
`loading` prop to correctly show/hide loading UI when prop value changes

- Updated dependencies
\[[`5ec70e688`](5ec70e6)]:
    -   @shopify/polaris-icons@9.0.1
    -   @shopify/polaris-tokens@9.0.1

## @shopify/polaris-icons@9.0.1

### Patch Changes

- [#11924](#11924)
[`5ec70e688`](5ec70e6)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to jest
29

## @shopify/polaris-migrator@1.0.1

### Patch Changes

- [#11924](#11924)
[`5ec70e688`](5ec70e6)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to jest
29


- [#11930](#11930)
[`b111629d7`](b111629)
Thanks [@jesstelford](https://github.com/jesstelford)! - Migrate
Storybook stories to CSF v3

- Updated dependencies
\[[`5ec70e688`](5ec70e6)]:
    -   @shopify/polaris-tokens@9.0.1
    -   @shopify/stylelint-polaris@16.0.1

## @shopify/polaris-tokens@9.0.1

### Patch Changes

- [#11924](#11924)
[`5ec70e688`](5ec70e6)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to jest
29

## @shopify/stylelint-polaris@16.0.1

### Patch Changes

- Updated dependencies
\[[`5ec70e688`](5ec70e6)]:
    -   @shopify/polaris-tokens@9.0.1

## polaris-for-vscode@1.0.1

### Patch Changes

- Updated dependencies
\[[`5ec70e688`](5ec70e6)]:
    -   @shopify/polaris-tokens@9.0.1

## polaris.shopify.com@1.0.4

### Patch Changes

- [#11924](#11924)
[`5ec70e688`](5ec70e6)
Thanks [@jesstelford](https://github.com/jesstelford)! - Upgrade to jest
29

- Updated dependencies
\[[`a83084b3b`](a83084b),
[`5ec70e688`](5ec70e6),
[`9ee700be6`](9ee700b),
[`bcd16df24`](bcd16df),
[`ce13c4366`](ce13c43),
[`364ada59e`](364ada5),
[`1fef06256`](1fef062),
[`45308c97a`](45308c9),
[`1539f0e7c`](1539f0e),
[`5a32a3ff6`](5a32a3f),
[`b111629d7`](b111629),
[`0a9b72721`](0a9b727)]:
    -   @shopify/polaris@13.2.0
    -   @shopify/polaris-icons@9.0.1
    -   @shopify/polaris-tokens@9.0.1

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restricting height of the popover in combobox
4 participants