Skip to content

BREAKING: Remove Button's tooltipGapSize prop#671

Merged
frankieyan merged 1 commit intomainfrom
ernesto/remove-button-tooltipGapSize
Aug 15, 2022
Merged

BREAKING: Remove Button's tooltipGapSize prop#671
frankieyan merged 1 commit intomainfrom
ernesto/remove-button-tooltipGapSize

Conversation

@gnapse
Copy link
Copy Markdown
Contributor

@gnapse gnapse commented Jun 30, 2022

Short description

The tooltipGapSize button prop was introduced in #636, but was challenged shortly afterwards, as not being a good course of action. See #636 (comment).

However, the reason for needing it was related to the fact that icon-only buttons are too aggressive in assuming they should use the aria-label for the tooltip if no tooltip is given. This PR makes it so that icon-only buttons only fall back to use aria-label as tooltip if the tooltip is really omitted (that is, the prop is not given at all, or given with value undefined). If the tooltip prop is given, but with a falsy/empty value, the implicit tooltip is now omitted, allowing consumers of the button to fully own the rendering of the tooltip, or to really suppress it if they really want to (with great power comes great responsibility).

So, from now on, instead of this:

// Not good
<Button
  icon={<SomeIcon />}
  aria-label="Click me"
  tooltipGapSize={8}
/>

We should use this:

// Good
<Tooltip content="Click me" gapSize={8}>
  <Button
    icon={<SomeIcon />}
    aria-label="Click me"
    tooltip={null}
  />
</Tooltip>

However, the most common use case (not needing to customize the tooltip), remains as simple as ever:

<Button
  icon={<SomeIcon />}
  aria-label="Click me"
/>

Or customizing only the tooltip content, but nothing else:

<Button
  icon={<SomeIcon />}
  aria-label="Click me"
  tooltip="Click me NOW"
/>

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

This is a breaking change, which implies a new major version release.

@gnapse gnapse requested a review from a team June 30, 2022 22:24
@gnapse gnapse self-assigned this Jun 30, 2022
@gnapse gnapse requested review from pedroalves0 and removed request for a team June 30, 2022 22:24
@frankieyan frankieyan force-pushed the ernesto/remove-button-tooltipGapSize branch from 3817a05 to 8670017 Compare August 15, 2022 14:11
@frankieyan frankieyan merged commit 48409e6 into main Aug 15, 2022
@frankieyan frankieyan deleted the ernesto/remove-button-tooltipGapSize branch August 15, 2022 14:21
Sign up for free to 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.

3 participants