blueprint icon indicating copy to clipboard operation
blueprint copied to clipboard

No scrollbar in Select component when menu has dividers

Open evansjohnson opened this issue 9 months ago • 5 comments

Fixes https://github.com/palantir/blueprint/issues/6780

Checklist

  • [ ] Includes tests
  • [ ] Update documentation

I don't think these are relevant for this PR

Changes proposed in this pull request:

For a menu divider inside of a select-popover, we already have applied a 5px padding inside the popover. This means to get to full popover width we need to apply the -5px margin to the outer list element, and therefor also undo the -5px margin on the menu divider.

This PR is a variation on https://github.com/palantir/blueprint/pull/6786, which correctly identified that the -5px margin on the menu divider is responsible for the scrollbar being added, but this PR additionally applies this -5px margin on the list element so that the divider continues to take up the full width.

Reviewers should focus on:

  • Any other styling options I'm not thinking of that may be negatively impacted
  • Any concerns of this fix itself being a breaking change, since this style issue has likely existed since https://github.com/palantir/blueprint/pull/6269 was merged almost a year ago?

Screenshot

Before Screenshot 2024-05-15 at 2 20 22 PM

After Screenshot 2024-05-15 at 2 19 48 PM *note right and bottom padding outside of list are now also removed. this seems to match what we do in the menu popover

evansjohnson avatar May 15 '24 18:05 evansjohnson

Generate changelog in packages/core/changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration, performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • [ ] Feature
  • [ ] Improvement
  • [ ] Fix
  • [ ] Break
  • [ ] Deprecation
  • [ ] Manual task
  • [ ] Migration

Description No scrollbar in Select component when menu has dividers


Generate changelog in packages/select/changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration, performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • [ ] Feature
  • [ ] Improvement
  • [ ] Fix
  • [ ] Break
  • [ ] Deprecation
  • [ ] Manual task
  • [ ] Migration

Description No scrollbar in Select component when menu has dividers


Check the box to generate changelog(s)

  • [ ] Generate changelog entry

changelog-app[bot] avatar May 15 '24 18:05 changelog-app[bot]

no select scrollbar with dividers

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github avatar May 15 '24 18:05 svc-palantir-github

alpha order

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github avatar May 15 '24 18:05 svc-palantir-github

I think going over the <Menu /> is necessary. Can you post the screenshot of it?

chaejunlee avatar May 15 '24 23:05 chaejunlee

There's no changes to the menu in this PR, except when inside a select popover

I checked for good measure and in the generated docs preview you can see the styling menu is unaffected: Screenshot 2024-05-16 at 5 28 33 PM

Note in https://github.com/palantir/blueprint/pull/6786 since the -5px margin is removed there is a slight visual regression in the screenshot where the border no longer goes to the edge of the menu, which seems to be what the intent of that style is.

evansjohnson avatar May 16 '24 21:05 evansjohnson

Thanks for the close look and good catch @gluxon!

I wrapped up this approach since it was easy to add back the margin one more time for the items, but given how much back and forth this is I may try the psuedo element approach quickly as well.

As a nice to have - it looks like there was a small stying weirdness where there is empty space padding to the left but not right of the selection. With fixing the scrollbar overflow this also sizes the menu items correctly to have padding on both sides.

develop

Screenshot 2024-06-18 at 8 55 35 AM Screenshot 2024-06-18 at 8 55 29 AM

this PR

Screenshot 2024-06-18 at 8 56 00 AM Screenshot 2024-06-18 at 8 56 07 AM

with scrollbars showing only when scrolling still looks good Screenshot 2024-06-18 at 9 04 10 AM Screenshot 2024-06-18 at 9 04 01 AM

evansjohnson avatar Jun 18 '24 12:06 evansjohnson

add side padding around selection highlight

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github avatar Jun 18 '24 13:06 svc-palantir-github

better approach?

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github avatar Jun 18 '24 13:06 svc-palantir-github

As a nice to have - it looks like there was a small stying weirdness where there is empty space padding to the left but not right of the selection. With fixing the scrollbar overflow this also sizes the menu items correctly to have padding on both sides.

Very nice! 😁

gluxon avatar Jun 19 '24 03:06 gluxon

Update packages/select/src/components/select/_select.scss

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

svc-palantir-github avatar Jun 19 '24 04:06 svc-palantir-github