Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Introduce Tooltip to SegmentedControlIconButton #5679

Merged
merged 11 commits into from
Feb 11, 2025

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Feb 10, 2025

To do:

Relates to:

This enables a tooltip by default on the SegmentedControl.IconButton. For safe rollout, this wraps the updates in a new feature flag, primer_react_segmented_control_tooltip which will be tracked in https://github.com/github/primer/issues/4775.

We can enable this feature flag for 1-2 weeks in dotcom, before we clean it up and formally GA.

I welcome others thoughts.

Changelog

New

  • Shows tooltip by default for SegmentedControl.IconButton.
  • Wraps changes in a new feature flag.

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

Merge checklist

Copy link

changeset-bot bot commented Feb 10, 2025

🦋 Changeset detected

Latest commit: c92f2d7

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 Minor

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

Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 10, 2025
Copy link
Contributor

github-actions bot commented Feb 10, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 105.57 KB (+0.04% 🔺)
packages/react/dist/browser.umd.js 106.04 KB (+0.14% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 10, 2025 16:10 Inactive

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@khiga8 khiga8 force-pushed the kh-segmented-control-tooltip branch from 58d5f8a to d0c3f0a Compare February 10, 2025 18:30

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@khiga8 khiga8 force-pushed the kh-segmented-control-tooltip branch from 3487280 to 0b6f964 Compare February 10, 2025 18:36
@github-actions github-actions bot requested a deployment to storybook-preview-5679 February 10, 2025 18:37 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 10, 2025 18:49 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 10, 2025 19:04 Inactive
@github-actions github-actions bot requested a deployment to storybook-preview-5679 February 10, 2025 19:19 Abandoned
@khiga8 khiga8 marked this pull request as ready for review February 10, 2025 19:21
@khiga8 khiga8 requested a review from a team as a code owner February 10, 2025 19:21
@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 10, 2025 19:31 Inactive
<SegmentedControlIconButtonStyled
aria-current={selected}
// If description is provided, we will use the tooltip to describe the button, so we need to keep the aria-label to label the button.
aria-label={description ? ariaLabel : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since several of the props are the same in line 79-89 should we make a const.

const rest = {
  aria-current: selected,
  sx: enabled ? undefined : getSegmentedControlButtonStyles({selected}),
  className: clsx(enabled && classes.Button, enabled && classes.IconButton),
  ...rest
}

We could also add children ^.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good thought! hmmmm this is a lot of duplication, but I'm thinking that it's maybe fine since we'll remove everything in the else in 1-2 weeks when we GA the code!

@TylerJDev
Copy link
Member

I'm wondering if we just want to ship it without using unsafeDisableTooltip? Utilizing the prop is definitely safer, so I think that route is good to do if you wanted to stick with it. If we did skip the prop, we could look at the existing instances in Dotcom to see if there's any irregularities to gain confidence before shipping.

We could also feature flag it and staff ship it soon after it's released in Dotcom. Either option is good!

Copy link
Member

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

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

LGTM! Only comments are how you'd want to ship it in #5679 (comment)

@@ -144,7 +144,7 @@
"description": "Whether the segment is selected. This is used for controlled SegmentedControls, and needs to be updated using the onChange handler on SegmentedControl."
},
{
"name": "selected",
"name": "size",
Copy link
Member

Choose a reason for hiding this comment

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

Oh no, this was a typo? 🫣 Good catch! Hope no one used this the wrong way 😅

Comment on lines 172 to 176
"name": "unsafeDisableTooltip",
"type": "boolean",
"defaultValue": "false",
"required": false
},
Copy link
Member

Choose a reason for hiding this comment

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

We could probably forgo this one, as it will only be temporary, and will (hopefully) only be used by us.

@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 10, 2025 21:35 Inactive
@khiga8 khiga8 marked this pull request as draft February 10, 2025 21:58
@github-actions github-actions bot temporarily deployed to storybook-preview-5679 February 11, 2025 13:51 Inactive
@primer-integration
Copy link

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

@khiga8
Copy link
Contributor Author

khiga8 commented Feb 11, 2025

I updated the changes to be wrapped by a new feature flag, after discussing some more with @TylerJDev:

@khiga8 khiga8 marked this pull request as ready for review February 11, 2025 15:42
Copy link
Member

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

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

Feature flag looks good! Reapproving! ✨

@khiga8 khiga8 added this pull request to the merge queue Feb 11, 2025
Merged via the queue into main with commit dbcb8f7 Feb 11, 2025
47 checks passed
@khiga8 khiga8 deleted the kh-segmented-control-tooltip branch February 11, 2025 16:51
@primer primer bot mentioned this pull request Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants