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

Jump to bottom

[collapsible][accordion] Add keepMounted prop #807

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

[collapsible][accordion] Add keepMounted prop #807

wants to merge 14 commits into from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Nov 6, 2024 •
edited
Loading

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 •
edited
Loading

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
mj12albert
mj12albert commented Nov 7, 2024
View reviewed changes
packages/mui-base/src/Collapsible/Panel/CollapsiblePanel.test.tsx Outdated
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 added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 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 20, 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 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomiks atomiks Awaiting requested review from atomiks atomiks is a code owner

@colmtuite colmtuite Awaiting requested review from colmtuite colmtuite is a code owner

@michaldudak michaldudak Awaiting requested review from michaldudak michaldudak is a code owner

@vladmoroz vladmoroz Awaiting requested review from vladmoroz

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[collapsible] SupportkeepMounted prop
4 participants
@mj12albert @mui-bot @vladmoroz @michaldudak