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.

Sign up for GitHub

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 RadioButton #2057

Merged
merged 9 commits into from
Apr 10, 2024
Merged

feat: add autoFocus to RadioButton #2057

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
659d0e6
feat: add autoFocus to RadioButton
chensara Apr 10, 2024
036a608
feat: add autoFocus to docgen
chensara Apr 10, 2024
890bcf7
feat: ensure autoFocus works on RadioButton
chensara Apr 10, 2024
c517add
refactor: remove unneeded useEffect
chensara Apr 10, 2024
3df1500
refactor: remove unneeded code from docgen
chensara Apr 10, 2024
0c52132
test: back to old snapshot
chensara Apr 10, 2024
6384410
feat: autoFocus default value should be undefined
chensara Apr 10, 2024
bf8ab59
test: move things back to what they were
chensara Apr 10, 2024
15d56cc
Merge branch 'master' into feat/chensar/add-autofocus-to-radiobutton
chensara Apr 10, 2024
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
16 changes: 15 additions & 1 deletion packages/core/src/components/RadioButton/RadioButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import cx from "classnames";
import React, { forwardRef, useCallback, useMemo, useRef } from "react";
import React, { forwardRef, useCallback, useEffect, useMemo, useRef } from "react";
import useMergeRef from "../../hooks/useMergeRef";
import Clickable from "../Clickable/Clickable";
import Text from "../Text/Text";
Expand Down Expand Up @@ -27,6 +27,8 @@ export interface RadioButtonProps extends VibeComponentProps {
value?: string;
/** A string specifying a name for the input control. This name is submitted along with the control's value when the form data is submitted. */
name?: string;
/** is autoFocus */
autoFocus?: boolean;
/** is disabled */
disabled?: boolean;
/** why the input is disabled */
Expand Down Expand Up @@ -64,6 +66,7 @@ const RadioButton: VibeComponent<RadioButtonProps, HTMLElement> & object = forwa
*/
radioButtonClassName,
disabled = false,
autoFocus = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed :)

disabledReason,
defaultChecked = false,
children,
Expand All @@ -80,6 +83,16 @@ const RadioButton: VibeComponent<RadioButtonProps, HTMLElement> & object = forwa
const inputRef = useRef<HTMLInputElement | null>();
const mergedRef = useMergeRef(ref, inputRef);
const overrideClassName = backwardCompatibilityForProperties([className, componentClassName]);

useEffect(() => {
if (!inputRef?.current || !autoFocus) {
return;
}

const animationFrame = requestAnimationFrame(() => inputRef.current.focus());
return () => cancelAnimationFrame(animationFrame);
}, [inputRef, autoFocus]);

const onChildClick = useCallback(() => {
if (disabled || !retainChildClick) return;
if (inputRef.current) {
Expand Down Expand Up @@ -114,6 +127,7 @@ const RadioButton: VibeComponent<RadioButtonProps, HTMLElement> & object = forwa
type="radio"
value={value}
name={name}
autoFocus={autoFocus}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@chensara let's do the following
use the native autoFocus instead of the useEffect.

  • let's release a prerelease
  • or just use yarn pack on monday-ui-react-core's packages/core and 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 run yarn link monday-ui-react-core afterwards on your MF of course
    Notice in this case that you need to run yarn build for 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

when I've checked with the autoFocus it seems like working

chensara marked this conversation as resolved.
Show resolved Hide resolved
disabled={disabled}
{...checkedProps}
onChange={onSelect}
Expand Down
35 changes: 35 additions & 0 deletions ...c/components/RadioButton/__tests__/__snapshots__/radioButton-snapshot-tests.jest.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,31 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`RadioButton renders correctly when autoFocus 1`] = `
<label
className="radioButton"
data-testid="radio-button"
>
<span
className="inputContainer"
>
<input
autoFocus={true}
className="input"
defaultChecked={false}
disabled={false}
name=""
type="radio"
value=""
/>
<span
className="control labelAnimation"
data-testid="radio-button-control"
/>
</span>

</label>
`;

exports[`RadioButton renders correctly when checked 1`] = `
<label
className="radioButton"
Expand All @@ -9,6 +35,7 @@ exports[`RadioButton renders correctly when checked 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
checked={true}
className="input"
disabled={false}
Expand All @@ -34,6 +61,7 @@ exports[`RadioButton renders correctly when default checked 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={true}
disabled={false}
Expand All @@ -59,6 +87,7 @@ exports[`RadioButton renders correctly when disabled 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={true}
Expand All @@ -84,6 +113,7 @@ exports[`RadioButton renders correctly when unchecked 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
checked={false}
className="input"
disabled={false}
Expand All @@ -109,6 +139,7 @@ exports[`RadioButton renders correctly with componentClassName 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={false}
Expand All @@ -134,6 +165,7 @@ exports[`RadioButton renders correctly with empty props 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={false}
Expand All @@ -159,6 +191,7 @@ exports[`RadioButton renders correctly with name 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={false}
Expand All @@ -184,6 +217,7 @@ exports[`RadioButton renders correctly with text 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={false}
Expand Down Expand Up @@ -214,6 +248,7 @@ exports[`RadioButton renders correctly with value 1`] = `
className="inputContainer"
>
<input
autoFocus={false}
className="input"
defaultChecked={false}
disabled={false}
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/components/RadioButton/__tests__/radioButton-snapshot-tests.jest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import React from "react";
import renderer from "react-test-renderer";
import RadioButton from "../RadioButton";

Expand Down Expand Up @@ -33,6 +32,11 @@ describe("RadioButton renders correctly", () => {
expect(tree).toMatchSnapshot();
});

it("when autoFocus", () => {
const tree = renderer.create(<RadioButton autoFocus />).toJSON();
expect(tree).toMatchSnapshot();
});

it("when unchecked", () => {
const tree = renderer.create(<RadioButton checked={false} />).toJSON();
expect(tree).toMatchSnapshot();
Expand Down
135 changes: 79 additions & 56 deletions packages/core/src/components/RadioButton/__tests__/radioButton-tests.jest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,76 +1,99 @@
import React from "react";
import "@testing-library/jest-dom";
import { fireEvent, render, cleanup, screen } from "@testing-library/react";
import RadioButton from "../RadioButton";

describe("RadioButton tests", () => {
const formName = "myForm";
const radiosName = "radios";

const option1Value = "1";
let onChangeMock1: jest.Mock;
let onChangeMock2: jest.Mock;
let onChangeMock3: jest.Mock;

const option1Text = "Option 1";
const option2Value = "2";
const option2Text = "Option 2";
const option3Value = "3";
const option3Text = "Option 3";

let onChangeMock1: jest.Mock;
let onChangeMock2: jest.Mock;
let onChangeMock3: jest.Mock;
describe("With one of the radio buttons is checked by default", () => {
beforeEach(() => {
const radiosName = "radios";

beforeEach(() => {
onChangeMock1 = jest.fn();
onChangeMock2 = jest.fn();
onChangeMock3 = jest.fn();
const option1Value = "1";
const option2Value = "2";
const option3Value = "3";
Copy link
Contributor

@YossiSaadi YossiSaadi Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


render(
<form name={formName}>
<RadioButton
name={radiosName}
value={option1Value}
text={option1Text}
onSelect={onChangeMock1}
defaultChecked={true}
/>
<RadioButton name={radiosName} value={option2Value} text={option2Text} onSelect={onChangeMock2} />
<RadioButton name={radiosName} value={option3Value} text={option3Text} onSelect={onChangeMock3} />
</form>
);
});
onChangeMock1 = jest.fn();
onChangeMock2 = jest.fn();
onChangeMock3 = jest.fn();

afterEach(() => {
cleanup();
});
render(
<form name={formName}>
<RadioButton
name={radiosName}
value={option1Value}
text={option1Text}
onSelect={onChangeMock1}
defaultChecked
/>
<RadioButton name={radiosName} value={option2Value} text={option2Text} onSelect={onChangeMock2} />
<RadioButton name={radiosName} value={option3Value} text={option3Text} onSelect={onChangeMock3} />
</form>
);
});

it("should default select 1st option", () => {
const option1: HTMLInputElement = screen.getByLabelText(option1Text);
const option2: HTMLInputElement = screen.getByLabelText(option2Text);
const option3: HTMLInputElement = screen.getByLabelText(option3Text);
expect(option1.checked).toBeTruthy();
expect(option2.checked).toBeFalsy();
expect(option3.checked).toBeFalsy();
});
afterEach(() => {
cleanup();
});

it("should select 2nd option", () => {
const option1: HTMLInputElement = screen.getByLabelText(option1Text);
const option2: HTMLInputElement = screen.getByLabelText(option2Text);
const option3: HTMLInputElement = screen.getByLabelText(option3Text);
fireEvent.click(option2);
expect(option1.checked).toBeFalsy();
expect(option2.checked).toBeTruthy();
expect(option3.checked).toBeFalsy();
});
it("should default select 1st option", () => {
const option1: HTMLInputElement = screen.getByLabelText(option1Text);
const option2: HTMLInputElement = screen.getByLabelText(option2Text);
const option3: HTMLInputElement = screen.getByLabelText(option3Text);
expect(option1.checked).toBeTruthy();
expect(option2.checked).toBeFalsy();
expect(option3.checked).toBeFalsy();
});

it("should call the onChange callback when clicked", () => {
const option2 = screen.getByLabelText(option2Text);
fireEvent.click(option2);
expect(onChangeMock2.mock.calls.length).toBe(1);
expect(onChangeMock2.mock.calls[0]).toBeTruthy();
it("should select 2nd option", () => {
const option1: HTMLInputElement = screen.getByLabelText(option1Text);
const option2: HTMLInputElement = screen.getByLabelText(option2Text);
const option3: HTMLInputElement = screen.getByLabelText(option3Text);
fireEvent.click(option2);
expect(option1.checked).toBeFalsy();
expect(option2.checked).toBeTruthy();
expect(option3.checked).toBeFalsy();
});

it("should call the onChange callback when clicked", () => {
const option2 = screen.getByLabelText(option2Text);
fireEvent.click(option2);
expect(onChangeMock2.mock.calls.length).toBe(1);
expect(onChangeMock2.mock.calls[0]).toBeTruthy();
});

it("should be the same text", () => {
const text = "test text";
const { getByText } = render(<RadioButton text={text} />);
const radioButtonComponentText = getByText(text);
expect(radioButtonComponentText).toBeTruthy();
});
});

it("should be the same text", () => {
const text = "test text";
const { getByText } = render(<RadioButton text={text} />);
const radioButtonComponentText = getByText(text);
expect(radioButtonComponentText).toBeTruthy();
describe("When all radio buttons are unchecked", () => {
it("should auto focus 2nd option", () => {
render(
<form name={formName}>
<RadioButton text={option1Text} />
<RadioButton text={option2Text} autoFocus />
<RadioButton text={option3Text} />
</form>
);

const option1: HTMLInputElement = screen.getByLabelText(option1Text);
const option2: HTMLInputElement = screen.getByLabelText(option2Text);
const option3: HTMLInputElement = screen.getByLabelText(option3Text);
expect(option1).not.toHaveFocus();
expect(option2).toHaveFocus();
expect(option3).not.toHaveFocus();
});
});
});
11 changes: 11 additions & 0 deletions packages/core/src/storybook/stand-alone-documentaion/playground/react-docgen-output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12104,6 +12104,17 @@
"computed": false
}
},
"autoFocus": {
"required": false,
"tsType": {
"name": "boolean"
},
"description": "autoFocus when component mount",
"defaultValue": {
"value": "false",
"computed": false
}
},
"disabled": {
"required": false,
"tsType": {
Expand Down