feat: add autoFocus to RadioButton #2057
Conversation
| @@ -114,6 +127,7 @@ const RadioButton: VibeComponent<RadioButtonProps, HTMLElement> & object = forwa | |||
| type="radio" | |||
| value={value} | |||
| name={name} | |||
| autoFocus={autoFocus} | |||
There was a problem hiding this comment.
I'm not sure if both methods are needed, the useEffect and the autoFocus.
I know that TextField implements it using the useEffect with the animation frame, which I guess is for a reason.
@talkor wdyt?
There was a problem hiding this comment.
Both should work but IMO it's better to just use the autoFocus prop which is an HTML attribute which makes the useEffect redundant in this case
There was a problem hiding this comment.
I agree, and it does work on Storybook, but they might implemented the useEffect as in some scenarios possibly there was a render issue requiring this to be on the next animation frame?
not sure
There was a problem hiding this comment.
@chensara let's do the following
use the native autoFocus instead of the useEffect.
- let's release a prerelease
- or just use
yarn packonmonday-ui-react-core'spackages/coreand use it in your local version - you can also use
yarn link, which is better IMO, with your MF.
Make sure that you run the command using the same node version that is used on your MF, in Vibe we're using node v18.12.1
Also make sure to runyarn link monday-ui-react-coreafterwards on your MF of course
Notice in this case that you need to runyarn buildfor every change to take effect on your MF
if it works for you with the native approach, amazing. if not, try the same with the useEffect
There was a problem hiding this comment.
So I first implemented using only autoFocus attr but it doesn't work in stroybook so I added the useEffect and it worked. Should I delete autoFocus attr?
There was a problem hiding this comment.
when I've checked with the autoFocus it seems like working
| @@ -64,6 +66,7 @@ const RadioButton: VibeComponent<RadioButtonProps, HTMLElement> & object = forwa | |||
| */ | |||
| radioButtonClassName, | |||
| disabled = false, | |||
| autoFocus = false, | |||
There was a problem hiding this comment.
maybe leave it undefined in case it isn't supplied
so current snapshots won't add autoFocus=false to their snapshots, as it can lead to - when upgrading a version in monolith - to multiple snapshots being broken
we're ok with it as undefined
so just remove the default false
| const radiosName = "radios"; | ||
|
|
||
| beforeEach(() => { | ||
| onChangeMock1 = jest.fn(); | ||
| onChangeMock2 = jest.fn(); | ||
| onChangeMock3 = jest.fn(); | ||
| const option1Value = "1"; | ||
| const option2Value = "2"; | ||
| const option3Value = "3"; |
There was a problem hiding this comment.
I'm not sure about inserting the *Value inside, and also about radiosName, sounds like they can lay outside of this block (even in the top of the file, outside of any block, but they can just get back to where they were before)
did something not worked for you while they were there?
In the new hard signup we want to autofocus the first option for each question in order to support accesibility and voice reading in the future: see -> task item
So for this reason, I added
autoFocusprop toRadioButton