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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 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
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
39 changes: 1 addition & 38 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,37 +91,6 @@ describe('<Checkbox.Indicator />', () => {
);
const indicator = container.querySelector('span');
expect(indicator).not.to.equal(null);
expect(indicator).not.to.have.attribute('hidden');
});
});

it('should remove the indicator when there is no exit animation defined', async ({ skip }) => {
if (isJSDOM) {
skip();
}

function Test() {
const [checked, setChecked] = React.useState(true);
return (
<div>
<button onClick={() => setChecked(false)}>Close</button>
<Checkbox.Root checked={checked}>
<Checkbox.Indicator data-testid="indicator" keepMounted />
</Checkbox.Root>
</div>
);
}

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

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

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

await user.click(closeButton);

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

Expand Down Expand Up @@ -173,15 +140,11 @@ describe('<Checkbox.Indicator />', () => {

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

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

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
42 changes: 1 addition & 41 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 @@ -28,44 +28,6 @@ describe('<Menu.CheckboxItemIndicator />', () => {
},
}));

it('should remove the indicator when there is no exit animation defined', async ({ skip }) => {
if (isJSDOM) {
skip();
}

function Test() {
const [checked, setChecked] = React.useState(true);
return (
<div>
<button onClick={() => setChecked(false)}>Close</button>
<Menu.Root open modal={false}>
<Menu.Portal>
<Menu.Positioner>
<Menu.Popup>
<Menu.CheckboxItem checked={checked}>
<Menu.CheckboxItemIndicator data-testid="indicator" keepMounted />
</Menu.CheckboxItem>
</Menu.Popup>
</Menu.Positioner>
</Menu.Portal>
</Menu.Root>
</div>
);
}

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

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

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

await user.click(closeButton);

await waitFor(() => {
expect(screen.getByTestId('indicator')).to.have.attribute('hidden');
});
Copy link
Contributor

@atomiks atomiks Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think each of these tests can be kept, just check for null instead of hidden (removing the keepMounted prop in the test)?

});

it('should remove the indicator when the animation finishes', async ({ skip }) => {
if (isJSDOM) {
skip();
Expand Down Expand Up @@ -125,9 +87,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
51 changes: 1 addition & 50 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 @@ -26,51 +26,6 @@ describe('<Menu.RadioItemIndicator />', () => {
},
}));

it('should remove the indicator when there is no exit animation defined', async ({ skip }) => {
if (isJSDOM) {
skip();
}

function Test() {
const [value, setValue] = React.useState('a');
return (
<div>
<button onClick={() => setValue('b')}>Close</button>
<Menu.Root open modal={false}>
<Menu.Portal>
<Menu.Positioner>
<Menu.Popup>
<Menu.Popup>
<Menu.RadioGroup value={value}>
<Menu.RadioItem value="a">
<Menu.RadioItemIndicator data-testid="indicator" keepMounted />
</Menu.RadioItem>
<Menu.RadioItem value="b">
<Menu.RadioItemIndicator keepMounted />
</Menu.RadioItem>
</Menu.RadioGroup>
</Menu.Popup>
</Menu.Popup>
</Menu.Positioner>
</Menu.Portal>
</Menu.Root>
</div>
);
}

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

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

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

await user.click(closeButton);

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

it('should remove the indicator when the animation finishes', async ({ skip }) => {
if (isJSDOM) {
skip();
Expand Down Expand Up @@ -129,15 +84,11 @@ describe('<Menu.RadioItemIndicator />', () => {

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

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

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
45 changes: 1 addition & 44 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 @@ -19,45 +19,6 @@ describe('<Radio.Indicator />', () => {
},
}));

it('should remove the indicator when there is no exit animation defined', async ({ skip }) => {
if (isJSDOM) {
skip();
}

function Test() {
const [value, setValue] = React.useState('a');
return (
<div>
<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.Root>
<Radio.Root value="a">
<Radio.Indicator className="animation-test-indicator" keepMounted />
</Radio.Root>
</RadioGroup>
</div>
);
}

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

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

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

await user.click(closeButton);

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

it('should remove the indicator when the animation finishes', async ({ skip }) => {
if (isJSDOM) {
skip();
Expand Down Expand Up @@ -109,15 +70,11 @@ describe('<Radio.Indicator />', () => {

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

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

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);
});
});
7 changes: 2 additions & 5 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 @@ -25,7 +25,7 @@ const RadioIndicator = React.forwardRef(function RadioIndicator(

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
Loading