Show focus on Button when interacting with keyboard
#244
Conversation
justinanastos
requested review from
cheapsteak,
Jephuff,
jgzuke and
jchesterman
src/Button/Button.tsx
Outdated
|
|
||
| mergedProps.onClick?.(event); | ||
| }, | ||
| [mergedProps] |
There was a problem hiding this comment.
mergeProps is going to be a new object every render, so this useCallback can go away.
Or if wanted to keep it, probably need to store the merged props in a ref?
There was a problem hiding this comment.
Great catch 👍
src/Button/Button.tsx
Outdated
| [mergedProps] | ||
| ); | ||
|
|
||
| const focusedStyles: ObjectInterpolation<any> = { |
There was a problem hiding this comment.
Looks like this defaults to undefined inside the type, could we use that instead of any?
| const focusedStyles: ObjectInterpolation<any> = { | |
| const focusedStyles: ObjectInterpolation<undefined> = { |
| @@ -447,7 +447,7 @@ In space constrained places or for floating action buttons (FAB), we can show an | |||
|
|
|||
| ### End Icons | |||
|
|
|||
| You can specify that a button has an additional icon at the end of the row | |||
| You can specify that a button has an additional icon at the end of the row. This will be assumed to be a menu selector and will change the button's right padding. | |||
There was a problem hiding this comment.
This seems a bit unexpected? I would expect icon and endIcon to behave similarly. If endIcon is for a specific use case maybe the name should reflect that.
Or remove endIcon and add a prop like isMenuSelect that changes the padding and moves the icon to the right?
There was a problem hiding this comment.
In retrospect I wish I'd named it something more like that; but this is already in place and now will just behave correctly 🤷♂️
justinanastos
force-pushed
the
justin/button-focus
branch
from
f0efeae
to
98a6cdc
Compare
justinanastos
force-pushed
the
justin/button-focus
branch
2 times, most recently
from
a4e7e33
to
030fbb5
Compare
This will _appear_ the same as before, but now the behavior is closer to correct. Before when a user clicked, we would blur the button to not show it's focused style. This was wrong for accessiblity because when a button is clicked, it should retain the focus to allow tabbing. Now we use `@react-aria/focus` to show the focus style if the user arrived on the element with the keyboard only.
This does the things we're doing by hand automatically.
justinanastos
force-pushed
the
justin/button-focus
branch
from
030fbb5
to
04f902a
Compare
|
🚀 PR was released in |
This uses
@react-aria/focusunder the hood to handle the focus. Also, this cleans up some internals of button.Release Notes
Buttonwhen keyboard focus is received@react-aria/utils'smergePropsinButtonButtonforclassNamemerging📦 Published PR as canary version:
7.17.1-canary.244.5608.0✨ Test out this PR locally via:
npm install @apollo/space-kit@7.17.1-canary.244.5608.0 # or yarn add @apollo/space-kit@7.17.1-canary.244.5608.0