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

Closed
wants to merge 14 commits into 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 1238a2e

@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 😓

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 26, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 26, 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 26, 2024
@mj12albert
Copy link
Member Author

Also includes a lot of rework to the panel internals as the Select PR changed the timing of the useTransitionStatus hook and set data-entering 1 animation frame sooner than before: atomiks@cfd185a#diff-ecb73c88ec0196a8b56a56bb2b147a4d57e6f461c854c51e7a3109408336fb6c

This broke CSS transition handling, as it was using that 1 frame between the open state changing and data-entering to measure the fully expanded size of the panel contents before applying the transition (which starts when data-entering is applied)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 28, 2024
@mj12albert mj12albert closed this Nov 28, 2024
@mj12albert
Copy link
Member Author

Continued in #889

Requires #887

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! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[collapsible] SupportkeepMounted prop
4 participants