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

TextInput trailing action design and API update #2033

Merged
merged 15 commits into from
Apr 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions .changeset/tall-dancers-exercise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@primer/react': patch
---

- Text input inner action's hover bg should not touch the text input edges
- Increases touch target area of the text input inner action button
- Deprecated `children` and `variant` props on the `TextInputInnerAction` component, but they're **still supported for now**.
49 changes: 17 additions & 32 deletions docs/content/TextInput.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -111,37 +111,21 @@ render(<WithIconAndLoadingIndicator />)
### With trailing action

```jsx live
<Box display="grid" sx={{gap: 3}}>
<FormControl>
<FormControl.Label>Icon action</FormControl.Label>
<TextInput
trailingAction={
<TextInput.Action
onClick={() => {
alert('clear input')
}}
icon={XIcon}
aria-label="Clear input"
sx={{color: 'fg.subtle'}}
/>
}
/>
</FormControl>
<FormControl>
<FormControl.Label>Text action</FormControl.Label>
<TextInput
trailingAction={
<TextInput.Action
onClick={() => {
alert('clear input')
}}
>
Clear
</TextInput.Action>
}
/>
</FormControl>
</Box>
<FormControl>
<FormControl.Label>Icon action</FormControl.Label>
<TextInput
trailingAction={
<TextInput.Action
onClick={() => {
alert('clear input')
}}
icon={XIcon}
aria-label="Clear input"
sx={{color: 'fg.subtle'}}
/>
}
/>
</FormControl>
```

### With error and warning states
Expand Down Expand Up @@ -315,7 +299,8 @@ render(<WithIconAndLoadingIndicator />)
<PropsTableRow
name="variant"
type="'default' | 'primary' | 'invisible' | 'danger'"
description="Determine's the styles on a button"
description="(Deprecated so that only the 'invisible' variant is used) Determine's the styles on a button"
deprecated
/>
<PropsTableBasePropRows
elementType="button"
Expand Down
4 changes: 4 additions & 0 deletions src/Button/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ export const getSizeStyles = (size = 'medium', variant: VariantType = 'default',
fontSize = 1
}
if (iconOnly) {
// when `size !== 'medium'`, vertical alignment of the icon is thrown off
// because changing the font size draws an em-box that does not match the
// bounding box of the SVG
fontSize = 1
paddingX = paddingY + 3 // to make it a square
}
if (variant === 'invisible') {
Expand Down
29 changes: 26 additions & 3 deletions src/_TextInputInnerAction.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,38 @@ import {ButtonProps} from './Button'
import {BetterSystemStyleObject, merge, SxProp} from './sx'

type TextInputActionProps = Omit<React.HTMLProps<HTMLButtonElement>, 'aria-label' | 'size'> & {
/** @deprecated Text input action buttons should only use icon buttons */
children?: React.ReactNode
/** Text that appears in a tooltip. If an icon is passed, this is also used as the label used by assistive technologies. */
['aria-label']?: string
/** The icon to render inside the button */
icon?: React.FunctionComponent<IconProps>
/**
* @deprecated Text input action buttons should only use the 'invisible' button variant
Copy link
Member

@siddharthkp siddharthkp Apr 22, 2022

Choose a reason for hiding this comment

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

Question: Does this make it a breaking change? Would there be new warnings when someone upgrades primer/react?

How do we usually deprecate props?

Side note: Are there any users of TextInput.Action? Can we get away with this in patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this make it a breaking change?

No, if they're still using variant, it will still work


Would there be new warnings when someone upgrades primer/react?

I think it would just be in their IDE


How do we usually deprecate props?

I don't think we have a defined process yet. Maybe @colebemis knows


Side note: Are there any users of TextInput.Action? Can we get away with this in patch

I don't think so. Even if there were, I think we can get away with this in a patch.

* Determine's the styles on a button one of 'default' | 'primary' | 'invisible' | 'danger'
*/
variant?: ButtonProps['variant']
} & SxProp

const invisibleButtonStyleOverrides = {
color: 'fg.default'
color: 'fg.default',
paddingTop: '2px',
paddingRight: '4px',
paddingBottom: '2px',
paddingLeft: '4px',
position: 'relative',

'@media (pointer: coarse)': {
':after': {
content: '""',
position: 'absolute',
left: 0,
right: 0,
transform: 'translateY(-50%)',
top: '50%',
minHeight: '44px'
}
}
}

const ConditionalTooltip: React.FC<{
Expand Down Expand Up @@ -44,22 +64,25 @@ const ConditionalTooltip: React.FC<{
const TextInputAction = forwardRef<HTMLButtonElement, TextInputActionProps>(
({'aria-label': ariaLabel, children, icon, sx: sxProp, variant, ...rest}, forwardedRef) => {
const sx =
variant === 'invisible' ? merge<BetterSystemStyleObject>(invisibleButtonStyleOverrides, sxProp || {}) : sxProp
variant === 'invisible'
? merge<BetterSystemStyleObject>(invisibleButtonStyleOverrides, sxProp || {})
: sxProp || {}

if ((icon && !ariaLabel) || (!children && !ariaLabel)) {
// eslint-disable-next-line no-console
console.warn('Use the `aria-label` prop to provide an accessible label for assistive technology')
}

return (
<Box as="span" className="TextInput-action">
<Box as="span" className="TextInput-action" margin={1}>
{icon && !children ? (
<Tooltip aria-label={ariaLabel}>
<IconButton
variant={variant}
type="button"
icon={icon}
aria-label={ariaLabel}
size="small"
sx={sx}
{...rest}
ref={forwardedRef}
Expand Down
Loading