Skip to content

TextInput: Update TextInput.Action internal component to use Tooltip v2 #4097

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.

Sign up for GitHub

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

Merged
broccolinisoup merged 8 commits into from
Mar 4, 2024
Merged

TextInput: Update TextInput.Action internal component to use Tooltip v2 #4097

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7ad2a86
Use Tooltip 2 for TextInput.Action
broccolinisoup Dec 21, 2023
0944f24
add changeset
broccolinisoup Dec 21, 2023
03773e6
snaps and update default direction
broccolinisoup Dec 21, 2023
f0a40f6
update tooltip direction and type casting
broccolinisoup Dec 22, 2023
8f40f8e
Merge branch 'main' into bs/textinput-tooltip2
broccolinisoup Jan 12, 2024
a9da0e5
accidently removed - bring it back
broccolinisoup Jan 15, 2024
b2a912e
Merge branch 'main' into bs/textinput-tooltip2
mperrotti Jan 18, 2024
c24979c
Merge branch 'main' into bs/textinput-tooltip2
broccolinisoup Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wet-geckos-accept.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

TextInput: Update TextInput.Action internal component to use Tooltip v2
6 changes: 3 additions & 3 deletions src/internal/components/TextInputInnerAction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, {forwardRef} from 'react'
import {IconProps} from '@primer/octicons-react'
import Box from '../../Box'
import {Button, IconButton, ButtonProps} from '../../Button'
import Tooltip from '../../Tooltip/Tooltip'
import {Tooltip} from '../../drafts/Tooltip'
import {BetterSystemStyleObject, merge, SxProp} from '../../sx'

type TextInputActionProps = Omit<
Expand Down Expand Up @@ -59,7 +59,7 @@ const ConditionalTooltip: React.FC<
<>
{ariaLabel ? (
<Tooltip
aria-label={ariaLabel}
text={ariaLabel}
direction={tooltipDirection}
sx={{
/* inline-block is used to ensure the tooltip dimensions don't
Expand Down Expand Up @@ -93,7 +93,7 @@ const TextInputAction = forwardRef<HTMLButtonElement, TextInputActionProps>(
return (
<Box as="span" className="TextInput-action" marginLeft={1} marginRight={1} lineHeight="0">
{icon && !children ? (
<Tooltip direction={tooltipDirection} aria-label={ariaLabel}>
<Tooltip direction={tooltipDirection} text={ariaLabel as unknown as string} type="label">

@broccolinisoup broccolinisoup Dec 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

text={ariaLabel as unknown as string} this is not great but some sort of type conversion or "hack" is needed here because text is a required prop for Tooltip but I see we can't make ariaLabel required for TextInput's trailing action because it could be an icon button or a text button. I am open to feedback if we want to handle this in another way

@mperrotti mperrotti Dec 21, 2023

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.

Would this be a little easier to read/understand?

text={ariaLabel || ''}

@broccolinisoup broccolinisoup Dec 22, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah that reads better. Thank you. Although both options will end up rendering an empty tooltip. I wonder if we should conditionally render the tooltip for icon button too? Though this solution will only work until we bring the tooltip to icon buttons by default #2008. What are your thoughts on that?

@mperrotti mperrotti Jan 18, 2024

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.

Ah ok. Yea, I think we can keep what you have then.

<IconButton
variant={variant}
type="button"
Expand Down