TextInput trailing action design and API update #2033
Conversation
🦋 Changeset detectedLatest commit: 9b9c963 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
…r-action-design-update
The tooltip container seems to be positioned awkwardly here. You can hover and get the tooltip but you're not on the button yet so a click doesn't trigger the button. Kapture.2022-04-19.at.18.28.42.mp4
24x24, does this meet our minimum button size for a11y? How do we solve for this when it's limited by the container's size?
|
Great catch! I'll fix that.
We increase the size of the clickable area for |
TIL! |
| /** 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/Button/IconButton.tsx
Outdated
| // 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}, |
There was a problem hiding this comment.
Option: Should we move this to getSizeStyles where fontSize is set?
Line 232 in 4fd9b74
.changeset/tall-dancers-exercise.md
Outdated
| - Inner action's hover bg should not touch the text input edges | ||
| - Increases touch target area of the inner action button | ||
| - Deprecation: we only want to allow inner action buttons to be icon buttons | ||
| - Deprecation: we only want to allow inner action buttons to be the 'invisible' variant |
There was a problem hiding this comment.
Optional: Should we rephrase to look more like a change log?
"we only want to allow inner action buttons to be icon buttons" sounds a bit rude in a changelog 😅
There was a problem hiding this comment.
Well that's no good. Changing!
…primer/react into mp/textinput-inner-action-design-update
…r-action-design-update
Changes:
Screenshots
Before:
After:
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.