re-add scrollbar to command

[Sebastian-ubs]

before

after

This change is 

[Sebastian-ubs]

The effective change is only removing tw:no-scrollbar

[imnasnainaec]

@imnasnainaec partially reviewed 11 files and all commit messages, and made 1 comment.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on Sebastian-ubs).

---

lib/platform-bible-react/src/components/shadcn-ui/command.tsx line 149 at r1 (raw file):
Devin observes

lib/platform-bible-react/src/components/shadcn-ui/command.tsx:R146-149
Scrollbar visibility change affects all CommandList consumers

Removing tw:no-scrollbar from CommandList (command.tsx:149) is a visual change that propagates to every consumer: combo-box.component.tsx, marker-menu.component.tsx, comment-editor.component.tsx, project-selector.component.tsx, recent-searches.component.tsx, book-chapter-control.component.tsx, multi-select-combo-box.component.tsx, select-books-picker.component.tsx, and overlay-command-palette.component.tsx.

Some of these (e.g. compact combo boxes or the command palette) may have been designed with the hidden-scrollbar aesthetic in mind. If any consumer needs the old behavior, it can pass className="tw:no-scrollbar" to override, so this is non-breaking — but worth a visual check across all consumers.

Have you evaluated whether tw:no-scrollbar should be added to any of the consumers that weren't the target of this change?

[imnasnainaec]

@imnasnainaec made 1 comment.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on Sebastian-ubs).

---

lib/platform-bible-react/src/components/shadcn-ui/command.tsx line 149 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Devin observes

lib/platform-bible-react/src/components/shadcn-ui/command.tsx:R146-149
Scrollbar visibility change affects all CommandList consumers

Removing tw:no-scrollbar from CommandList (command.tsx:149) is a visual change that propagates to every consumer: combo-box.component.tsx, marker-menu.component.tsx, comment-editor.component.tsx, project-selector.component.tsx, recent-searches.component.tsx, book-chapter-control.component.tsx, multi-select-combo-box.component.tsx, select-books-picker.component.tsx, and overlay-command-palette.component.tsx.

Some of these (e.g. compact combo boxes or the command palette) may have been designed with the hidden-scrollbar aesthetic in mind. If any consumer needs the old behavior, it can pass className="tw:no-scrollbar" to override, so this is non-breaking — but worth a visual check across all consumers.

Have you evaluated whether tw:no-scrollbar should be added to any of the consumers that weren't the target of this change?

When pushed, Devin is only concerned about the command palette and recommends a side-by-side visual comparison.

[Sebastian-ubs]

Yes, the intention is to push it to all of them

[imnasnainaec]

@imnasnainaec reviewed 1 file and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.

[merchako]

Concern: on macOS, it's normal for the app to not add scrollbars because our native environment adds them in ways that respect OS settings. There's an (unconfirmed) risk that this change might override that behavior, @Sebastian-ubs

[merchako]

Also, change height to be an amount that's not divisible by the row height—that will default the Illusion of completeness

[Sebastian-ubs]

@merchako TLDR: this fix just aligns command-based components with the rest of the app and as an unplanned fix cannot change anything on top at the moment.

I agree that row height and OS specific settings would be a win, but this is not what this fix is focusing on. Instead this fix is for "there are no scrollbars visible at all, ever" for this component(s). Better handling of scrollbars should be targeted across the app, where it also happens in other places (e.g. menus, panels, popovers, dialogs, ...), but this is only a little point fix for all command-based components.

Not sure if macOS does currently show scrollbars there on hovering (this is what we finally want across OSes, but needs more general implementation). At least Windows - because of the css class - does not show any scrollbar on that component ever and this is what the current fix is about.

[merchako]

Ok, go ahead then