[Checkbox, Menu, and Radio] Fix: Update keepMounted Prop Logic for Inline Elements #1329
Conversation
onehanddev
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
Netlify deploy preview |
…onehanddev/base-ui into fix/keepMounted-inline-elements
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
zannager
added
component: menu
|
I was wondering—do we still need the hidden prop? It seems the only scenario where it was set to true was when the keepMounted prop was used on the Indicator. Now that we’re removing that logic, I’m unsure if it’s still necessary. |
Nope, it can be removed entirely for the indicator components |
…dified test cases accordingly
| await waitFor(() => { | ||
| expect(screen.getByTestId('indicator')).to.have.attribute('hidden'); | ||
| }); |
There was a problem hiding this comment.
I think each of these tests can be kept, just check for null instead of hidden (removing the keepMounted prop in the test)?
| <Radio.Indicator | ||
| className="animation-test-indicator" | ||
| keepMounted | ||
| data-testid="indicator-a" | ||
| /> |
There was a problem hiding this comment.
@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 ?
There was a problem hiding this comment.
Thanks for spotting - that's unintended. All indicators should unmount by default
There was a problem hiding this comment.
@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.
…ent exists instead of its hidden state, set keepMounted to false by default for RadioIndicator like the rest of the Indicator components.
Overview
This pull request addresses the behavior of the
keepMountedprop across several components, ensuring that thehiddenattribute is not applied whenkeepMountedis set totrue. This change affects theCheckboxIndicator,MenuCheckboxItemIndicator,MenuRadioItemIndicator, andRadioIndicatorcomponents.Fixes #1298