[Button Group] Add outlined prop #6788

ljlinsc · 2024-05-06T01:18:14Z

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

Add outlined prop to ButtonGroup component.

Screenshot

No outline	Outlined (new changes)	Regular Button

changelog-app · 2024-05-06T01:18:18Z

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

[Button Group] Add outlined prop

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

[Button Group] Add outlined prop

Check the box to generate changelog(s)

Generate changelog entry

svc-palantir-github · 2024-05-06T01:39:34Z

Fix lint error

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

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

adidahiya · 2024-05-07T12:29:59Z

Nit: I think the border colors might need adjustments, the vertical borders between buttons are slightly more opaque (brighter in dark theme, darker in light theme) than the outer border of the group:

Invalidated by push of 25aeb89

svc-palantir-github · 2024-05-07T15:55:53Z

Fix border overlap

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

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

evansjohnson

just one small comment - could you push a commit so the PR preview builds again?

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

evansjohnson · 2024-07-03T14:38:29Z

packages/core/src/components/button/buttonGroup.tsx

        const buttonGroupClasses = classNames(
            Classes.BUTTON_GROUP,
            {
                [Classes.FILL]: fill,
                [Classes.LARGE]: large,
                [Classes.MINIMAL]: minimal,
+                [Classes.OUTLINED]: outlined,


svc-palantir-github · 2024-07-03T15:22:29Z

Only apply border-bottom inside outlined wrapper

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

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

evansjohnson

leaving this review just in case it's helpful in the future - @ljlinsc highlighted #4026 so I don't think we should worry about the slight px difference for now since it's also an issue in the underlying button component

evansjohnson · 2024-07-03T15:35:00Z

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

+    }
+  }
+
+  &:not(.#{$ns}-minimal):not(.#{$ns}-outlined) {


is this needed? I'm seeing the button group grow in size when outlined which I don't think is intentional

(added some extra buttons locally to test)

vertical (extra 1px per button, expect for last is what I'd assume):

horizontal (2px per button):

evansjohnson · 2024-07-03T15:44:00Z

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

+
+    &.#{$ns}-outlined {
+      > .#{$ns}-button:not(:last-child) {
+        border-bottom: none;


could we do a transparent color border instead of none here? border-bottom-color: transparent - it looks like that should fix any small shifts when toggling this prop

evansjohnson · 2024-07-03T15:44:53Z

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

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


here I think none instead of transparent color actually helps, since the Button component itself currently grows 2px when outlined. this brings that down to 1px at least

evansjohnson

Thank you for the PR!

We may need to revisit some of this CSS if addressing #4026 but this seems fine for now. The underlying button also displays some px shift when toggling this prop, so not worrying about it here either for now.

In practice if users use outline consistently (don't toggle for the same button group) this shouldn't be a big issue.

michael-yx-wu previously approved these changes May 6, 2024

View reviewed changes

ericjeney assigned evansjohnson Jul 3, 2024

evansjohnson reviewed Jul 3, 2024

View reviewed changes

Leslie Lin and others added 4 commits July 3, 2024 10:42

[Button Group] Add outlined prop

247ab74

Add generated changelog entries

05c1d30

Fix lint error

328f55c

Fix border overlap

3dfbeab

ljlinsc force-pushed the llin/button-group-outlined branch 3 times, most recently from 38c31cd to 8f61358 Compare July 3, 2024 15:07

Only apply border-bottom inside outlined wrapper

b70d7d2

ljlinsc force-pushed the llin/button-group-outlined branch from 8f61358 to b70d7d2 Compare July 3, 2024 15:08

evansjohnson reviewed Jul 15, 2024

View reviewed changes

evansjohnson approved these changes Jul 15, 2024

View reviewed changes

ljlinsc merged commit b38194b into develop Jul 15, 2024
13 checks passed

ljlinsc deleted the llin/button-group-outlined branch July 15, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button Group] Add outlined prop #6788

[Button Group] Add outlined prop #6788

ljlinsc commented May 6, 2024 •

edited

Loading

changelog-app bot commented May 6, 2024 •

edited by ljlinsc

Loading

svc-palantir-github commented May 6, 2024

adidahiya commented May 7, 2024

svc-palantir-github commented May 7, 2024

evansjohnson left a comment

evansjohnson Jul 3, 2024

svc-palantir-github commented Jul 3, 2024

evansjohnson left a comment

evansjohnson Jul 3, 2024

evansjohnson Jul 3, 2024

evansjohnson Jul 3, 2024

evansjohnson left a comment

[Button Group] Add outlined prop #6788

[Button Group] Add outlined prop #6788

Conversation

ljlinsc commented May 6, 2024 • edited Loading

Checklist

Changes proposed in this pull request:

Screenshot

changelog-app bot commented May 6, 2024 • edited by ljlinsc Loading

Generate changelog in packages/core/changelog/@unreleased

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

svc-palantir-github commented May 6, 2024

Fix lint error

adidahiya commented May 7, 2024

svc-palantir-github commented May 7, 2024

Fix border overlap

evansjohnson left a comment

Choose a reason for hiding this comment

evansjohnson Jul 3, 2024

Choose a reason for hiding this comment

svc-palantir-github commented Jul 3, 2024

Only apply border-bottom inside outlined wrapper

evansjohnson left a comment

Choose a reason for hiding this comment

evansjohnson Jul 3, 2024

Choose a reason for hiding this comment

evansjohnson Jul 3, 2024

Choose a reason for hiding this comment

evansjohnson Jul 3, 2024

Choose a reason for hiding this comment

evansjohnson left a comment

Choose a reason for hiding this comment

ljlinsc commented May 6, 2024 •

edited

Loading

changelog-app bot commented May 6, 2024 •

edited by ljlinsc

Loading

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

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