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

[Checkbox, Menu, and Radio] Fix: Update keepMounted Prop Logic for Inline Elements #1329

Merged
merged 11 commits into from
Jan 22, 2025
Merged

[Checkbox, Menu, and Radio] Fix: Update keepMounted Prop Logic for Inline Elements #1329

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c09a5d9
fix: Ensure keepMounted prop prevents hidden attribute for inline ele…
onehanddev Jan 13, 2025
cd2a8dd
Merge branch 'master' into fix/keepMounted-inline-elements
onehanddev Jan 13, 2025
2fdd28f
fix: modified test to accomodate new changes
onehanddev Jan 13, 2025
2fa4c1c
Merge branch 'fix/keepMounted-inline-elements' of https://github.com/…
onehanddev Jan 13, 2025
df77283
fix: prettier
onehanddev Jan 13, 2025
cfbb532
fix: removed hidden prop support from all indicator components and mo…
onehanddev Jan 16, 2025
d31dcd8
fix: reverted unwanted file commit
onehanddev Jan 16, 2025
2848aac
fix: modified indicator component tests to check to see if the compon…
onehanddev Jan 20, 2025
e952ad1
fix: updated JSDoc for previous change
onehanddev Jan 20, 2025
0cb08ff
Merge branch 'master' into fix/keepMounted-inline-elements
onehanddev Jan 20, 2025
ed69f33
fix: jsdoc
onehanddev Jan 21, 2025
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
2 changes: 1 addition & 1 deletion docs/reference/generated/radio-indicator.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
},
"keepMounted": {
"type": "boolean",
"default": "true",
"default": "false",
"description": "Whether to keep the HTML element in the DOM when the radio button is inactive."
},
"render": {
Expand Down
16 changes: 5 additions & 11 deletions packages/react/src/checkbox/indicator/CheckboxIndicator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ describe('<Checkbox.Indicator />', () => {
);
const indicator = container.querySelector('span');
expect(indicator).not.to.equal(null);
expect(indicator).to.have.attribute('hidden');
});

it('should keep indicator mounted when checked', async () => {
Expand All @@ -82,7 +81,6 @@ describe('<Checkbox.Indicator />', () => {
);
const indicator = container.querySelector('span');
expect(indicator).not.to.equal(null);
expect(indicator).not.to.have.attribute('hidden');
});

it('should keep indicator mounted when indeterminate', async () => {
Expand All @@ -93,7 +91,6 @@ describe('<Checkbox.Indicator />', () => {
);
const indicator = container.querySelector('span');
expect(indicator).not.to.equal(null);
expect(indicator).not.to.have.attribute('hidden');
});
});

Expand All @@ -108,22 +105,22 @@ describe('<Checkbox.Indicator />', () => {
<div>
<button onClick={() => setChecked(false)}>Close</button>
<Checkbox.Root checked={checked}>
<Checkbox.Indicator data-testid="indicator" keepMounted />
<Checkbox.Indicator data-testid="indicator" />
</Checkbox.Root>
</div>
);
}

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator')).not.to.have.attribute('hidden');
expect(screen.getByTestId('indicator')).not.to.equal(null);

const closeButton = screen.getByText('Close');

await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(screen.queryByTestId('indicator')).to.equal(null);
});
});

Expand Down Expand Up @@ -172,16 +169,13 @@ describe('<Checkbox.Indicator />', () => {
}

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator')).not.to.have.attribute('hidden');
expect(screen.getByTestId('indicator')).not.to.equal(null);

const closeButton = screen.getByText('Close');
await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(animationFinished).to.equal(true);
});

expect(animationFinished).to.equal(true);
});
});
7 changes: 2 additions & 5 deletions packages/react/src/checkbox/indicator/CheckboxIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const CheckboxIndicator = React.forwardRef(function CheckboxIndicator(

const rendered = rootState.checked || rootState.indeterminate;

const { mounted, transitionStatus, setMounted } = useTransitionStatus(rendered);
const { transitionStatus, setMounted } = useTransitionStatus(rendered);

const indicatorRef = React.useRef<HTMLSpanElement | null>(null);
const mergedRef = useForkRef(forwardedRef, indicatorRef);
Expand Down Expand Up @@ -65,10 +65,7 @@ const CheckboxIndicator = React.forwardRef(function CheckboxIndicator(
state,
className,
customStyleHookMapping,
extraProps: {
hidden: !mounted,
...otherProps,
},
extraProps: otherProps,
});

const shouldRender = keepMounted || rendered;
Expand Down
10 changes: 4 additions & 6 deletions packages/react/src/menu/checkbox-item-indicator/MenuCheckboxItemIndicator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('<Menu.CheckboxItemIndicator />', () => {
<Menu.Positioner>
<Menu.Popup>
<Menu.CheckboxItem checked={checked}>
<Menu.CheckboxItemIndicator data-testid="indicator" keepMounted />
<Menu.CheckboxItemIndicator data-testid="indicator" />
</Menu.CheckboxItem>
</Menu.Popup>
</Menu.Positioner>
Expand All @@ -55,14 +55,14 @@ describe('<Menu.CheckboxItemIndicator />', () => {

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator')).not.to.have.attribute('hidden');
expect(screen.queryByTestId('indicator')).not.to.equal(null);

const closeButton = screen.getByText('Close');

await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(screen.queryByTestId('indicator')).to.equal(null);
});
});

Expand Down Expand Up @@ -125,9 +125,7 @@ describe('<Menu.CheckboxItemIndicator />', () => {
await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(animationFinished).to.equal(true);
});

expect(animationFinished).to.equal(true);
});
});
5 changes: 2 additions & 3 deletions packages/react/src/menu/checkbox-item-indicator/MenuCheckboxItemIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@ const MenuCheckboxItemIndicator = React.forwardRef(function MenuCheckboxItemIndi
const indicatorRef = React.useRef<HTMLSpanElement | null>(null);
const mergedRef = useForkRef(forwardedRef, indicatorRef);

const { mounted, transitionStatus, setMounted } = useTransitionStatus(item.checked);
const { transitionStatus, setMounted } = useTransitionStatus(item.checked);

const getItemProps = React.useCallback(
(externalProps = {}) =>
mergeReactProps(externalProps, {
'aria-hidden': true,
hidden: !mounted,
}),
[mounted],
[],
);

useAfterExitAnimation({
Expand Down
12 changes: 5 additions & 7 deletions packages/react/src/menu/radio-item-indicator/MenuRadioItemIndicator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('<Menu.RadioItemIndicator />', () => {
<Menu.Popup>
<Menu.RadioGroup value={value}>
<Menu.RadioItem value="a">
<Menu.RadioItemIndicator data-testid="indicator" keepMounted />
<Menu.RadioItemIndicator data-testid="indicator" />
</Menu.RadioItem>
<Menu.RadioItem value="b">
<Menu.RadioItemIndicator keepMounted />
Expand All @@ -60,14 +60,14 @@ describe('<Menu.RadioItemIndicator />', () => {

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator')).not.to.have.attribute('hidden');
expect(screen.queryByTestId('indicator')).not.to.equal(null);

const closeButton = screen.getByText('Close');

await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(screen.queryByTestId('indicator')).to.equal(null);
});
});

Expand Down Expand Up @@ -129,15 +129,13 @@ describe('<Menu.RadioItemIndicator />', () => {

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator')).not.to.have.attribute('hidden');
expect(screen.getByTestId('indicator')).not.to.equal(null);

const closeButton = screen.getByText('Close');
await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
expect(animationFinished).to.equal(true);
});

expect(animationFinished).to.equal(true);
});
});
5 changes: 2 additions & 3 deletions packages/react/src/menu/radio-item-indicator/MenuRadioItemIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@ const MenuRadioItemIndicator = React.forwardRef(function MenuRadioItemIndicator(
const indicatorRef = React.useRef<HTMLSpanElement | null>(null);
const mergedRef = useForkRef(forwardedRef, indicatorRef);

const { mounted, transitionStatus, setMounted } = useTransitionStatus(item.checked);
const { transitionStatus, setMounted } = useTransitionStatus(item.checked);

const getItemProps = React.useCallback(
(externalProps = {}) =>
mergeReactProps(externalProps, {
'aria-hidden': true,
hidden: !mounted,
}),
[mounted],
[],
);

useAfterExitAnimation({
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/radio-group/RadioGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,10 @@ describe('<RadioGroup />', () => {
render(
<RadioGroup>
<Radio.Root value="a" data-testid="a">
<Radio.Indicator data-testid="indicator-a" />
<Radio.Indicator keepMounted data-testid="indicator-a" />
</Radio.Root>
<Radio.Root value="b" data-testid="b">
<Radio.Indicator data-testid="indicator-b" />
<Radio.Indicator keepMounted data-testid="indicator-b" />
</Radio.Root>
</RadioGroup>,
);
Expand Down
18 changes: 6 additions & 12 deletions packages/react/src/radio/indicator/RadioIndicator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,10 @@ describe('<Radio.Indicator />', () => {
<button onClick={() => setValue('b')}>Close</button>
<RadioGroup value={value}>
<Radio.Root value="a">
<Radio.Indicator
className="animation-test-indicator"
keepMounted
data-testid="indicator-a"
/>
Comment on lines -34 to -38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atomiks while reverting the test cases removal commit, I have found that the keepMounted will always be true for Radio.Indicator component (refer: radio/indicator/RadioIndicator.tsx), which is not the case for other components, is this an expected behaviour or unintended ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for spotting - that's unintended. All indicators should unmount by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atomiks I set it to false by default, like other indicator components. I also reverted the test cases you suggested earlier. Instead of checking for hidden props, I now check if the indicator element exists. Please check the latest commit and let me know if any modifications are needed.

<Radio.Indicator className="animation-test-indicator" data-testid="indicator-a" />
</Radio.Root>
<Radio.Root value="a">
<Radio.Indicator className="animation-test-indicator" keepMounted />
<Radio.Indicator className="animation-test-indicator" />
</Radio.Root>
</RadioGroup>
</div>
Expand All @@ -47,14 +43,14 @@ describe('<Radio.Indicator />', () => {

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator-a')).not.to.have.attribute('hidden');
expect(screen.getByTestId('indicator-a')).not.to.equal(null);

const closeButton = screen.getByText('Close');

await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator-a')).to.have.attribute('hidden');
expect(screen.queryByTestId('indicator-a')).to.equal(null);
});
});

Expand Down Expand Up @@ -109,15 +105,13 @@ describe('<Radio.Indicator />', () => {

const { user } = await render(<Test />);

expect(screen.getByTestId('indicator-a')).not.to.have.attribute('hidden');
expect(screen.getByTestId('indicator-a')).not.to.equal(null);

const closeButton = screen.getByText('Close');
await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator-a')).to.have.attribute('hidden');
expect(animationFinished).to.equal(true);
});

expect(animationFinished).to.equal(true);
});
});
11 changes: 4 additions & 7 deletions packages/react/src/radio/indicator/RadioIndicator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ const RadioIndicator = React.forwardRef(function RadioIndicator(
props: RadioIndicator.Props,
forwardedRef: React.ForwardedRef<HTMLSpanElement>,
) {
const { render, className, keepMounted = true, ...otherProps } = props;
const { render, className, keepMounted = false, ...otherProps } = props;

const rootState = useRadioRootContext();

const rendered = rootState.checked;

const { mounted, transitionStatus, setMounted } = useTransitionStatus(rendered);
const { transitionStatus, setMounted } = useTransitionStatus(rendered);

const state: RadioIndicator.State = React.useMemo(
() => ({
Expand All @@ -43,10 +43,7 @@ const RadioIndicator = React.forwardRef(function RadioIndicator(
ref: mergedRef,
className,
state,
extraProps: {
hidden: !mounted,
...otherProps,
},
extraProps: otherProps,
customStyleHookMapping,
});

Expand Down Expand Up @@ -100,7 +97,7 @@ RadioIndicator.propTypes /* remove-proptypes */ = {
className: PropTypes.oneOfType([PropTypes.func, PropTypes.string]),
/**
* Whether to keep the HTML element in the DOM when the radio button is inactive.
* @default true
* @default false
*/
keepMounted: PropTypes.bool,
/**
Expand Down
Loading