search

iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
105 stars 38 forks source link

`.iui-scroll` (`DropdownMenu`) should have maxHeight #733

Closed r100-stack closed 2 years ago

r100-stack commented 2 years ago

In the interest of not blocking this PR:

  • Lets add an inline style that sets max-height to 315 (same as our other dropdown components).
  • Lets open an issue in the iTwinUI repo to handle max-height there.

_Originally posted by @mayank99 in https://github.com/iTwin/iTwinUI-react/pull/756#discussion_r938891896_

The .iui-scroll class (DropdownMenu) currently doesn't have a max height. The suggestion is to put a max height of 315px to the .iui-scroll class (DropdownMenu)

veekeys commented 2 years ago

I dont think we can put max-height on iui-scroll class as menu is reused in different components. i.s. Select might have different max-height than menu in Table. Unless you want to add max-height in the component itself (Table).

mayank99 commented 2 years ago

I dont think we can put max-height on iui-scroll class as menu is reused in different components. i.s. Select might have different max-height than menu in Table. Unless you want to add max-height in the component itself (Table).

Select, ComboBox and now columnManager all use 315px as a hardcoded inline style. What's the problem with adding it here? https://github.com/iTwin/iTwinUI/blob/c6e6dc977c2791d4ac4ef7498b3495cd5ef6d5c6/packages/itwinui-css/src/menu/menu.scss#L28-L31

veekeys commented 2 years ago

I dont think we can put max-height on iui-scroll class as menu is reused in different components. i.s. Select might have different max-height than menu in Table. Unless you want to add max-height in the component itself (Table).

Select, ComboBox and now columnManager all use 315px as a hardcoded inline style. What's the problem with adding it here?

https://github.com/iTwin/iTwinUI/blob/c6e6dc977c2791d4ac4ef7498b3495cd5ef6d5c6/packages/itwinui-css/src/menu/menu.scss#L28-L31

Hardcoded number does not sound good. For Table I would imagine it should be relative to Table height instead of being hardcoded (which can change depending on the density type too). So I would just think we cannot give a single hardcoded number for all the cases we will reuse Menu. If we ever add small size to menu item, then it also should influence that... Instead, having a max-height per component looks like a better approach. That way we could keep menu still generic.

Do we have specificity issues if user wants to change max-height ?

mayank99 commented 2 years ago

What about a CSS variable then? We can call it --iui-menu-height or something, and we can give it a default value of 315px and change it dynamically in the future if we want.

The specificity will be 0-2-0 with the current css structure, but that's better than an inline style which overrides everything.

r100-stack commented 2 years ago

PR #740 closes this issue