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

[collapsible][accordion] Add keepMounted prop #807

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

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Nov 6, 2024

Closes #728

We decided to default keepMounted to false for consistency with other components.

Since hiddenUntilFound requires keepMounted={true} to work, using it will override keepMounted and show a warning in dev mode if both hiddenUntilFound and keepMounted={false} are specified.

@mj12albert mj12albert added component: collapsible This is the name of the generic UI component, not the React module! component: accordion This is the name of the generic UI component, not the React module! labels Nov 6, 2024
@mui-bot
Copy link

mui-bot commented Nov 6, 2024

Netlify deploy preview

https://deploy-preview-807--base-ui.netlify.app/

Generated by 🚫 dangerJS against 0a3d8d1

@mj12albert mj12albert force-pushed the feat/collapsible/keep-mounted branch 11 times, most recently from 33cd50b to 4778c4a Compare November 7, 2024 08:03
@mj12albert mj12albert marked this pull request as ready for review November 7, 2024 08:27
@mj12albert
Copy link
Member Author

@colmtuite @vladmoroz Do you think it's an issue that 2 props are required to use hidden-until-found now?

<Collapsible.Panel hiddenUntilFound keepMounted>
  {/* content */}
</Collapsible.Panel>

@vladmoroz
Copy link
Contributor

@mj12albert I think we should always mount the panel when hiddenUntilFound is used, regardless of keepMounted

@michaldudak
Copy link
Member

If keepMounted is undefined, then yes, we can make hiddenUntilFound make the component always mounted. But when it's explicitly set to false, I'd show a warning (in dev mode only, though).

@mj12albert mj12albert force-pushed the feat/collapsible/keep-mounted branch 2 times, most recently from f57a122 to 11b782d Compare November 7, 2024 13:11
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2024
@mj12albert mj12albert force-pushed the feat/collapsible/keep-mounted branch 3 times, most recently from f9f677f to 06c64f4 Compare November 7, 2024 14:17
Comment on lines +116 to +118
// toWarnDev doesn't work reliably with async rendering. To re-enable after it's fixed in the test-utils.
// eslint-disable-next-line mocha/no-skipped-tests
it.skip('warns when setting both `hiddenUntilFound` and `keepMounted={false}`', async function test() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this warning manually 😓

@mj12albert
Copy link
Member Author

Updated ~ would appreciate a review @vladmoroz @michaldudak 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: accordion This is the name of the generic UI component, not the React module! component: collapsible This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[collapsible] SupportkeepMounted prop
4 participants