Skip to content

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

Merged
broccolinisoup merged 6 commits into
mainfrom
button-group-tooltip-style-issues
Feb 8, 2024
Merged

ButtonGroup: Fix button and icon button styling when tooltips are used on them#4210
broccolinisoup merged 6 commits into
mainfrom
button-group-tooltip-style-issues

Conversation

@broccolinisoup

@broccolinisoupbroccolinisoup commented Feb 2, 2024

Copy link
Copy Markdown
Member

Closes#4129

Changelog

New

Changed

Updated Button group CSS to make sure the icon buttons and buttons are rendered as expected when they have tooltips (both v1 and v2) 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

@broccolinisoup
broccolinisoup requested review from a team and siddharthkpFebruary 2, 2024 03:57
@changeset-bot

changeset-botBot commented Feb 2, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f427dfd

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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

github-actionsBot commented Feb 2, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js113.01 KB (+0.06% 🔺)
packages/react/dist/browser.umd.js113.66 KB (+0.05% 🔺)

Comment threadsrc/Button/IconButton.features.stories.tsx Outdated
@broccolinisoup
broccolinisoupforce-pushed the button-group-tooltip-style-issues branch from f04ef75 to 3b0c065CompareFebruary 2, 2024 04:11
</ButtonGroup>
)

export const IconButtonsWithTooltip = () => (

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to remove them after the review if we don't want to keep them

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave em' :)

@langermanklangermank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small question, otherwise looks good 👍

</ButtonGroup>
)

export const IconButtonsWithTooltip = () => (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave em' :)

Comment threadsrc/ButtonGroup/ButtonGroup.tsx Outdated
isolation: isolate;

&& > * {
&& > button,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@broccolinisoup is it possible these could be a tags as well? You could add a test story case for that and see.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oo great catch @langermank! Definitely, they can be a tags. I'll update the code.

@broccolinisoup

This comment was marked as off-topic.

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4210 February 7, 2024 02:30 Inactive
@broccolinisoupbroccolinisoup added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Feb 7, 2024
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Feb 7, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4210 February 7, 2024 02:46 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4210 February 8, 2024 02:15 Inactive
@broccolinisoup
broccolinisoup added this pull request to the merge queueFeb 8, 2024
Merged via the queue into main with commit 3acbd80Feb 8, 2024
@broccolinisoup
broccolinisoup deleted the button-group-tooltip-style-issues branch February 8, 2024 02:53
@primerprimerBot mentioned this pull request Feb 8, 2024
broccolinisoup added a commit that referenced this pull request Feb 9, 2024
github-merge-queueBot pushed a commit that referenced this pull request Feb 11, 2024
broccolinisoup added a commit that referenced this pull request Feb 11, 2024
…d on them (#4210)
* fix button group styling for tooltip and tooltip v2
* Add changeset
* take the a tag into account when styling
* add e2e tests for the buttons groups that use tooltip
* test(vrt): update snapshots
---------
Co-authored-by: broccolinisoup <broccolinisoup@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using <Tooltip> within <ButtonGroup> affects styling

3 participants

@broccolinisoup@joshblack@langermank