Skip to content

setting: Make dropdown popup scrollable by default with fluent override#2293

Merged
madcodelife merged 1 commit intolongbridge:mainfrom
boboshan:feat/setting-dropdown-scrollable
Apr 27, 2026
Merged

setting: Make dropdown popup scrollable by default with fluent override#2293
madcodelife merged 1 commit intolongbridge:mainfrom
boboshan:feat/setting-dropdown-scrollable

Conversation

@boboshan
Copy link
Copy Markdown
Contributor

@boboshan boboshan commented Apr 25, 2026 •
edited
Loading

Summary

SettingField's dropdown variant builds its popup via PopupMenu but never opts into PopupMenu::scrollable. With long option lists the menu overflows the viewport and entries below the fold are unreachable.

This PR:

  • Defaults scrollable: true so out-of-the-box dropdowns degrade gracefully on long menus.
  • Adds a fluent .scrollable(bool) setter on SettingField<SharedString> for callers who want to opt out, mirroring the builder style of GroupBoxVariants::with_variant and Sizable::small/medium.
SettingField::dropdown(items, getter, setter)              // scrollable
SettingField::dropdown(items, getter, setter)
    .scrollable(false)                                      // no scroll

SettingFieldType::Dropdown gains a scrollable: bool field; the existing dropdown(...) constructor sets it to true. The setter mutates the variant in place when the field is a dropdown, no-op otherwise.

No public API breakage — all existing callers of SettingField::dropdown(...) keep working unchanged.

Test plan

  • cargo check --workspace passes locally.
  • Long option list (verified in a downstream app): menu scrolls, items below fold reachable.
  • Short option list still renders without scrollbar chrome.
  • dropdown(...).scrollable(false) disables scrolling.

madcodelife
madcodelife reviewed Apr 27, 2026
View reviewed changes
Comment thread crates/ui/src/setting/fields/mod.rs Outdated
/// the viewport, default is `true`.
///
/// Has no effect on non-dropdown field types.
pub fn scrollable(mut self, scrollable: bool) -> Self {
Copy link
Copy Markdown
Member

@madcodelife madcodelife Apr 27, 2026

Choose a reason for hiding this comment

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

Thanks for addressing the overflow issue.

The scrollable(bool) method is somewhat opaque — it would be better to just add a dedicated scrollable_dropdown() method to distinguish it from the existing dropdown().

SettingField::dropdown(items, getter, setter)            // unchanged, no scroll
SettingField::scrollable_dropdown(items, getter, setter) // opt-in scroll

@boboshan @claude
setting: Add scrollable_dropdown constructor for SettingField
8dec6dd 
`SettingField::dropdown` builds its popup via `PopupMenu` but never
opts into `PopupMenu::scrollable`. With long option lists the menu
overflows the viewport and entries below the fold are unreachable.

Add a sibling constructor:

```rust
SettingField::dropdown(items, getter, setter)            // unchanged
SettingField::scrollable_dropdown(items, getter, setter) // opt-in scroll
```

`SettingFieldType::Dropdown` gains a `scrollable: bool` field that the
two constructors set explicitly. Callers pick the variant at the call
site rather than via a fluent setter — which matches the suggestion on
the review.

`dropdown(...)` keeps `scrollable = false`, so existing call sites are
byte-identical. The new `scrollable_dropdown(...)` is purely additive.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@boboshan
Copy link
Copy Markdown
Contributor Author

boboshan commented Apr 27, 2026

Rewrote per the suggestion — replaced the fluent .scrollable(bool) setter with a sibling constructor:

SettingField::dropdown(items, ...)            // unchanged, no scroll
SettingField::scrollable_dropdown(items, ...) // opt-in scroll

dropdown(...) now keeps scrollable: false, so existing call sites are byte-identical. The new constructor is purely additive. Tested locally — short dropdowns stay non-scrolling, long ones (21-item theme picker) scroll cleanly. Force-pushed.

@boboshan boboshan force-pushed the feat/setting-dropdown-scrollable branch from 65ee4df to 8dec6dd Compare April 27, 2026 05:08
@madcodelife
Copy link
Copy Markdown
Member

madcodelife commented Apr 27, 2026

Thank you!

@madcodelife madcodelife merged commit f25b8c6 into longbridge:main Apr 27, 2026
3 checks passed
boboshan added a commit to boboshan/gpui-component that referenced this pull request Apr 27, 2026
@boboshan @claude
setting: Make dropdown popup scrollable by default with fluent overri…
3169b40 
…de (longbridge#2293)

## Summary

`SettingField`'s dropdown variant builds its popup via `PopupMenu` but
never opts into `PopupMenu::scrollable`. With long option lists the menu
overflows the viewport and entries below the fold are unreachable.

This PR:

- Defaults `scrollable: true` so out-of-the-box dropdowns degrade
gracefully on long menus.
- Adds a fluent `.scrollable(bool)` setter on
`SettingField<SharedString>` for callers who want to opt out, mirroring
the builder style of `GroupBoxVariants::with_variant` and
`Sizable::small`/`medium`.

```rust
SettingField::dropdown(items, getter, setter)              // scrollable
SettingField::dropdown(items, getter, setter)
    .scrollable(false)                                      // no scroll
```

`SettingFieldType::Dropdown` gains a `scrollable: bool` field; the
existing `dropdown(...)` constructor sets it to `true`. The setter
mutates the variant in place when the field is a dropdown, no-op
otherwise.

No public API breakage — all existing callers of
`SettingField::dropdown(...)` keep working unchanged.

## Test plan

- [x] `cargo check --workspace` passes locally.
- [x] Long option list (verified in a downstream app): menu scrolls,
items below fold reachable.
- [x] Short option list still renders without scrollbar chrome.
- [ ] `dropdown(...).scrollable(false)` disables scrolling.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@boboshan boboshan deleted the feat/setting-dropdown-scrollable branch April 27, 2026 05:46
boboshan added a commit to boboshan/gpui-component that referenced this pull request Apr 27, 2026
@boboshan @claude
setting: Make dropdown popup scrollable by default with fluent overri…
705768f 
…de (longbridge#2293)

## Summary

`SettingField`'s dropdown variant builds its popup via `PopupMenu` but
never opts into `PopupMenu::scrollable`. With long option lists the menu
overflows the viewport and entries below the fold are unreachable.

This PR:

- Defaults `scrollable: true` so out-of-the-box dropdowns degrade
gracefully on long menus.
- Adds a fluent `.scrollable(bool)` setter on
`SettingField<SharedString>` for callers who want to opt out, mirroring
the builder style of `GroupBoxVariants::with_variant` and
`Sizable::small`/`medium`.

```rust
SettingField::dropdown(items, getter, setter)              // scrollable
SettingField::dropdown(items, getter, setter)
    .scrollable(false)                                      // no scroll
```

`SettingFieldType::Dropdown` gains a `scrollable: bool` field; the
existing `dropdown(...)` constructor sets it to `true`. The setter
mutates the variant in place when the field is a dropdown, no-op
otherwise.

No public API breakage — all existing callers of
`SettingField::dropdown(...)` keep working unchanged.

## Test plan

- [x] `cargo check --workspace` passes locally.
- [x] Long option list (verified in a downstream app): menu scrolls,
items below fold reachable.
- [x] Short option list still renders without scrollbar chrome.
- [ ] `dropdown(...).scrollable(false)` disables scrolling.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AzureZee pushed a commit to AzureZee/gpui-component that referenced this pull request Apr 29, 2026
@boboshan @claude @AzureZee
setting: Make dropdown popup scrollable by default with fluent overri…
7e872ac 
…de (longbridge#2293)

## Summary

`SettingField`'s dropdown variant builds its popup via `PopupMenu` but
never opts into `PopupMenu::scrollable`. With long option lists the menu
overflows the viewport and entries below the fold are unreachable.

This PR:

- Defaults `scrollable: true` so out-of-the-box dropdowns degrade
gracefully on long menus.
- Adds a fluent `.scrollable(bool)` setter on
`SettingField<SharedString>` for callers who want to opt out, mirroring
the builder style of `GroupBoxVariants::with_variant` and
`Sizable::small`/`medium`.

```rust
SettingField::dropdown(items, getter, setter)              // scrollable
SettingField::dropdown(items, getter, setter)
    .scrollable(false)                                      // no scroll
```

`SettingFieldType::Dropdown` gains a `scrollable: bool` field; the
existing `dropdown(...)` constructor sets it to `true`. The setter
mutates the variant in place when the field is a dropdown, no-op
otherwise.

No public API breakage — all existing callers of
`SettingField::dropdown(...)` keep working unchanged.

## Test plan

- [x] `cargo check --workspace` passes locally.
- [x] Long option list (verified in a downstream app): menu scrolls,
items below fold reachable.
- [x] Short option list still renders without scrollbar chrome.
- [ ] `dropdown(...).scrollable(false)` disables scrolling.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madcodelife madcodelife madcodelife left review comments

Assignees

@madcodelife madcodelife

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@boboshan @madcodelife