Fix border overlap between Buttons in outlined ButtonGroup #6966

ggdouglas · 2024-09-10T01:12:50Z

Fixes #0000

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

Fixes an issue where outlined button styles were not rendering properly when wrapped in a Popover inside of ButtonGroup. Also adds an outlined variant prop example to the "Usage with popovers" ButtonGroup example to allow for visual testing of this issue in the future.

New `outlined` prop toggle on ButtonGroup's "Usage with popovers" example.

This change fixes borders on outlined buttons with popovers in two instances:

The first is where the outlined prop is applied to the ButtonGroup parent container:

<ButtonGroup outlined>
  <Popover content={...}>
    <Button icon="git-branch" rightIcon="caret-down" text="Main" />
  </Popover>
  <Button icon="more" />
</ButtonGroup>

In this case, buttons surrounded by popovers weren't being styled as outlined at all. The .bp5-popover-target class breaks the selector applying the outlined styling.

Before	After

The other is where the outlined prop is applied to the individual buttons within the group

<ButtonGroup>
  <Popover content={...}>
    <Button icon="git-branch" rightIcon="caret-down" text="Main" outlined />
  </Popover>
  <Button icon="more" outlined />
</ButtonGroup>

In this case, the outline styles render, but there is a slight overlap between the intermediate borders of the button group. This overlap is noticeable due to the semi-transparent styling of the borders on outlined buttons.

Reviewers should focus on:

Various possible Button variants/permutations that can be contained within a ButtonGroup wrapper
Buttons surrounded by Popovers
Buttons surrounded by Tooltips

changelog-app · 2024-09-10T01:12:55Z

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

Description

Fix border overlap between Buttons in outlined ButtonGroup

Generate changelog in `packages/docs-app/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

Description

Fix border overlap between Buttons in outlined ButtonGroup

Check the box to generate changelog(s)

Generate changelog entry

svc-palantir-github · 2024-09-10T01:27:03Z

Fix overlapping borders between buttons in outlined button group

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

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

invliD · 2024-09-10T17:26:30Z

packages/core/src/components/button/_button-group.scss

-    > .#{$ns}-button {
+    .#{$ns}-button {


We likely need to keep the child combinator here, otherwise a button inside an inline overlay within the button group would be affected by these styles. We'll need to be specific for popovers, similar to L100-L112 above.

Hm, good point. I'd neglected to test for inline overlays since it's a bit more involved to set up an example for those. Does this same issue not apply to the &.#{$ns}-minimal, &.#{$ns}-outlined selector above also?

blueprint/packages/core/src/components/button/_button-group.scss

Lines 121 to 122 in 1038d7f

&.#{$ns}-minimal, &.#{$ns}-outlined {

.#{$ns}-button {

Added back child combinator and selected for popovers here: 84f92db

invliD · 2024-09-10T17:27:01Z

packages/core/src/components/button/_button-group.scss

      @include pt-button-outlined();
    }

    &:not(.#{$ns}-vertical) {
+      > .#{$ns}-popover-target:not(:last-child) .#{$ns}-button,


invliD · 2024-09-10T17:27:33Z

packages/core/src/components/button/_button-group.scss

+  &:not(.#{$ns}-vertical) {
+    > .#{$ns}-popover-target:not(:last-child) .#{$ns}-button.#{$ns}-outlined,
+    > .#{$ns}-button.#{$ns}-outlined:not(:last-child) {
+      border-right: none;
+    }
+  }


Do we need this for vertical button groups too (with border-bottom)?

Ah, good catch! Added in 79e3441

svc-palantir-github · 2024-09-10T19:15:33Z

Handle vertical button groups with outlined button children

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

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

svc-palantir-github · 2024-09-10T19:28:33Z

Keep child combinator, explicitly select for popover target children

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

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

invliD · 2024-09-11T16:49:29Z

packages/core/src/components/popover/_popover-in-button-group.scss

+    }
+
+    &:not(.#{$ns}-outlined) {
+      > .#{$ns}-popover-target:not(:last-child) .#{$ns}-button:not(.#{$ns}-outlined) {


There seems to be a lot of duplicate (or at least similar) styles for popovers here and in the other file. We should probably consolidate them and also merge the selectors for popover-target and popover-wrapper, since they should likely apply the exact same styles (the latter only existing for back-compat anyway).

Yeah, I noticed this as well, but was hesitant to tackle reorganizing those styles in this PR since it's a more involved change. I'm also not super familiar with the historical use of the popover-wrapper class, so that also made the prospect of that change seem more brittle to me.

Happy to address cleaning these up though. Would you recommend this PR or a FLUP?

…6966)" This reverts commit 5410094.

ggdouglas added 2 commits September 9, 2024 20:12

Add outlined example variant to button group popover example

f89f0ec

Fix overlapping borders between buttons in outlined button group

7066a7f

ggdouglas force-pushed the gdouglas/fix-button-group-outlined branch from 7b20efb to 7066a7f Compare September 10, 2024 01:13

ericjeney assigned invliD Sep 10, 2024

invliD reviewed Sep 10, 2024

View reviewed changes

ggdouglas added 2 commits September 10, 2024 15:01

Handle vertical button groups with outlined button children

79e3441

Keep child combinator, explicitly select for popover target children

84f92db

ggdouglas requested a review from invliD September 11, 2024 01:17

invliD reviewed Sep 11, 2024

View reviewed changes

invliD approved these changes Sep 16, 2024

View reviewed changes

ggdouglas merged commit 5410094 into develop Sep 16, 2024
12 of 15 checks passed

ggdouglas deleted the gdouglas/fix-button-group-outlined branch September 16, 2024 18:51

ggdouglas mentioned this pull request Sep 25, 2024

Buttons in ButtonGroup missing borders when mixed Button types are used #7000

Open

ggdouglas added a commit that referenced this pull request Sep 27, 2024

Revert "Fix border overlap between Buttons in outlined ButtonGroup (#…

67006cc

…6966)" This reverts commit 5410094.

This was referenced Sep 27, 2024

Revert: Fix border overlap between Buttons in outlined ButtonGroup #7002

Merged

Collapse borders between buttons in outlined ButtonGroup #7005

Open

ggdouglas mentioned this pull request Dec 13, 2024

Apply outline styles to outlined ButtonGroups with Popover children #7129

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix border overlap between Buttons in outlined ButtonGroup #6966

Fix border overlap between Buttons in outlined ButtonGroup #6966

ggdouglas commented Sep 10, 2024 •

edited

Loading

changelog-app bot commented Sep 10, 2024

svc-palantir-github commented Sep 10, 2024

invliD Sep 10, 2024

ggdouglas Sep 10, 2024

invliD Sep 10, 2024

invliD Sep 10, 2024

ggdouglas Sep 10, 2024

svc-palantir-github commented Sep 10, 2024

svc-palantir-github commented Sep 10, 2024

invliD Sep 11, 2024

ggdouglas Sep 11, 2024

Fix border overlap between Buttons in outlined ButtonGroup #6966

Fix border overlap between Buttons in outlined ButtonGroup #6966

Conversation

ggdouglas commented Sep 10, 2024 • edited Loading

Fixes #0000

Checklist

Changes proposed in this pull request:

New outlined prop toggle on ButtonGroup's "Usage with popovers" example.

Reviewers should focus on:

changelog-app bot commented Sep 10, 2024

Generate changelog in packages/core/changelog/@unreleased

Generate changelog in packages/docs-app/changelog/@unreleased

svc-palantir-github commented Sep 10, 2024

Fix overlapping borders between buttons in outlined button group

invliD Sep 10, 2024

Choose a reason for hiding this comment

ggdouglas Sep 10, 2024

Choose a reason for hiding this comment

invliD Sep 10, 2024

Choose a reason for hiding this comment

invliD Sep 10, 2024

Choose a reason for hiding this comment

ggdouglas Sep 10, 2024

Choose a reason for hiding this comment

svc-palantir-github commented Sep 10, 2024

Handle vertical button groups with outlined button children

svc-palantir-github commented Sep 10, 2024

Keep child combinator, explicitly select for popover target children

invliD Sep 11, 2024

Choose a reason for hiding this comment

ggdouglas Sep 11, 2024

Choose a reason for hiding this comment

ggdouglas commented Sep 10, 2024 •

edited

Loading

New `outlined` prop toggle on ButtonGroup's "Usage with popovers" example.

Generate changelog in `packages/core/changelog/@unreleased`

Generate changelog in `packages/docs-app/changelog/@unreleased`