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

feat: add autoFocus to CheckBox #2059

Merged

Conversation

chensara
Copy link
Contributor

@chensara chensara commented Apr 10, 2024

In the new hard signup we want to autofocus the first option for each question in order to support accessibility and voice reading in the future: see -> task item
So for this reason, I added autoFocus prop to CheckBox

@chensara chensara requested a review from a team as a code owner April 10, 2024 18:14
@@ -122,7 +134,8 @@ describe("Checkbox tests", () => {
option1Value,
checkbox1Name,
checkbox2Name,
checkbox3Name
checkbox3Name,
defaultChecked: true
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the existing tests, the tests had defaultChecked set to true, I needed it to be false in my tests so I can check the autoFocus so I added this as optional

@@ -39,6 +40,8 @@ type RenderHelper = {
option3Text: string;
option3Value: string;
onChangeMock3: jest.MockedFunction<MockedFunction>;
defaultChecked?: boolean;
autoFocusOption2?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

autoFocusOption2 ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D, I can send just autoFocus and put it on the first option but I'm not sure it will be clear from outside.. WDYT?

@chensara chensara requested a review from talkor April 11, 2024 05:36
Copy link
Contributor

@YossiSaadi YossiSaadi left a comment

Choose a reason for hiding this comment

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

Seems cool. let's just remove the extra duplicated tests (unless there's a reason they're there, waiting for your reply)

@YossiSaadi YossiSaadi merged commit cd26e7c into mondaycom:master Apr 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants