ButtonGroup: Fix button and icon button styling when tooltips are used on them #4723

broccolinisoup · 2024-07-08T01:15:41Z

Closes #4129

Changelog

New

Changed

With this PR, we are wrapping button group items with a div. The main reason is to simplify CSS selection. You can view the previous attempt to write a CSS to fix styling issues both for tooltip v1 and v2 on button group.
Enhanced CSS selection for the ButtonGroup children elements to ensure buttons and links still look the same if there are tooltips on them

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

…ltips

changeset-bot · 2024-07-08T01:15:45Z

🦋 Changeset detected

Latest commit: 08df44e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-07-08T01:19:27Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	92.49 KB (+0.09% 🔺)
packages/react/dist/browser.umd.js	92.72 KB (+0.05% 🔺)

primer-integration · 2024-07-11T05:22:58Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/332533

mperrotti

Looks great! I just made some suggestions to simplify the styling a little.

Just confirm that my suggestions actually work and then I'll approve.

mperrotti · 2024-07-23T20:16:05Z

script/generate-e2e-tests.js

@@ -293,6 +293,38 @@ const components = new Map([
          id: 'components-buttongroup-features--icon-buttons',
          name: 'Icon Buttons',
        },
+        {
+          id: 'components-buttongroup-features--icon-buttons-with-tooltip',


Is this the new way we prefer to create VRTs? I've been manually editing the .test.ts files 😅

mperrotti · 2024-07-23T20:25:53Z

packages/react/src/ButtonGroup/ButtonGroup.tsx

  display: inline-flex;
  vertical-align: middle;
  isolation: isolate;

-  && > * {
+  && :is(button, a) {


I don't actually think we need :is for this. I think we can just select button and a directly

Suggested change

&& :is(button, a) {

button,

a {

mperrotti · 2024-07-23T20:26:58Z

packages/react/src/ButtonGroup/ButtonGroup.tsx

    :focus,
    :active,
    :hover {
      z-index: 1;
    }
  }

-  ${sx};
+  && > :first-child :is(button, a) {


Same here - I don't think we need && or :is, we can just directly select button and a children of :first-child

Suggested change

&& > :first-child :is(button, a) {

> :first-child button,

> :first-child a {

mperrotti · 2024-07-23T20:27:21Z

packages/react/src/ButtonGroup/ButtonGroup.tsx

+    border-bottom-left-radius: ${get('radii.2')};
+  }
+
+  && > :last-child :is(button, a) {


One more :)

Suggested change

&& > :last-child :is(button, a) {

> :last-child button,

> :last-child a {

broccolinisoup and others added 23 commits March 8, 2024 12:29

Explore wrapping button group items with a div to simplify css selection

784d775

fix type

49784c9

Merge branch 'main' into explore-wrapping-button-group-items

2ac0412

Merge branch 'main' into explore-wrapping-button-group-items

6a50500

Merge branch 'main' into explore-wrapping-button-group-items

2f70a76

Merge branch 'main' into explore-wrapping-button-group-items

13fff82

Merge branch 'main' into explore-wrapping-button-group-items

2c8af92

Merge branch 'main' into explore-wrapping-button-group-items

1ff84fd

Merge branch 'main' into explore-wrapping-button-group-items

4ce67b2

fix stroybook type

5dcd503

remove button group util tests from deprecated button

d71ae30

linting

d4eda64

make it a forward ref component

43e8e49

rest and ref on the parent element not the children

94c66fa

Merge branch 'main' into explore-wrapping-button-group-items

7abb1be

Add e2e tests

9496530

test(vrt): update snapshots

e3b2d82

disable tooltip animations on playwright

eab4812

test(vrt): update snapshots

7c962bf

disable animations

52c4828

test(vrt): update snapshots

8842661

Merge branch 'main' into explore-wrapping-button-group-items

c9415df

use unsafeDisableTooltip prop for iconbuttons and remove explicit too…

ea8b026

…ltips

broccolinisoup changed the title ~~tooltip button group styling fix~~ ButtonGroup: Fix button and icon button styling when tooltips are used on them Jul 8, 2024

broccolinisoup mentioned this pull request Jul 8, 2024

ButtonGroup: Fix button and icon button styling when tooltips are used on them #4371

Closed

13 tasks

broccolinisoup deployed to github-pages July 8, 2024 01:18 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4371 July 8, 2024 01:19 View deployment

broccolinisoup temporarily deployed to github-pages July 8, 2024 01:19 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4723 July 8, 2024 01:20 Inactive

add prx example for tmp

ace1542

broccolinisoup temporarily deployed to github-pages July 11, 2024 03:39 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4723 July 11, 2024 03:40 Inactive

Create dirty-cameras-smell.md

aa2a45d

broccolinisoup temporarily deployed to github-pages July 11, 2024 04:30 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4723 July 11, 2024 04:31 Inactive

Merge branch 'main' into tooltip-button-group-styling-fix

aa14d04

broccolinisoup marked this pull request as ready for review July 11, 2024 05:27

broccolinisoup requested review from a team as code owners July 11, 2024 05:27

broccolinisoup requested review from emilybrick and mperrotti July 11, 2024 05:27

broccolinisoup temporarily deployed to github-pages July 11, 2024 05:31 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4723 July 11, 2024 05:31 Inactive

Merge branch 'main' into tooltip-button-group-styling-fix

89aca5c

broccolinisoup temporarily deployed to github-pages July 16, 2024 05:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4723 July 16, 2024 05:14 Inactive

Merge branch 'main' into tooltip-button-group-styling-fix

08df44e

broccolinisoup deployed to github-pages July 23, 2024 01:02 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4723 July 23, 2024 01:02 View deployment

broccolinisoup requested a review from langermank July 23, 2024 02:17

mperrotti reviewed Jul 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ButtonGroup: Fix button and icon button styling when tooltips are used on them #4723

ButtonGroup: Fix button and icon button styling when tooltips are used on them #4723

broccolinisoup commented Jul 8, 2024 •

edited

Loading

changeset-bot bot commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

primer-integration bot commented Jul 11, 2024

mperrotti left a comment

mperrotti Jul 23, 2024

mperrotti Jul 23, 2024

mperrotti Jul 23, 2024

mperrotti Jul 23, 2024

	&& > :first-child :is(button, a) {
	> :first-child button,
	> :first-child a {

	&& > :last-child :is(button, a) {
	> :last-child button,
	> :last-child a {

ButtonGroup: Fix button and icon button styling when tooltips are used on them #4723

Are you sure you want to change the base?

ButtonGroup: Fix button and icon button styling when tooltips are used on them #4723

Conversation

broccolinisoup commented Jul 8, 2024 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Jul 8, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 8, 2024 • edited Loading

size-limit report 📦

primer-integration bot commented Jul 11, 2024

mperrotti left a comment

Choose a reason for hiding this comment

mperrotti Jul 23, 2024

Choose a reason for hiding this comment

mperrotti Jul 23, 2024

Choose a reason for hiding this comment

mperrotti Jul 23, 2024

Choose a reason for hiding this comment

mperrotti Jul 23, 2024

Choose a reason for hiding this comment

broccolinisoup commented Jul 8, 2024 •

edited

Loading

changeset-bot bot commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading