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] Add Collapsible components and hooks #481

Merged
merged 12 commits into from
Aug 28, 2024

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Jul 17, 2024

@mj12albert mj12albert added new feature New feature or request component: collapsible This is the name of the generic UI component, not the React module! labels Jul 17, 2024
@mui-bot
Copy link

mui-bot commented Jul 17, 2024

Netlify deploy preview

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

Generated by 🚫 dangerJS against 58810db

@mj12albert mj12albert force-pushed the feat/collapsible-component branch 2 times, most recently from 69f291b to 397ff68 Compare July 17, 2024 12:13
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 25, 2024
@mj12albert mj12albert force-pushed the feat/collapsible-component branch from 397ff68 to 30e8cc0 Compare July 29, 2024 10:57
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2024
@mj12albert mj12albert force-pushed the feat/collapsible-component branch 5 times, most recently from e78e435 to 3638e68 Compare August 6, 2024 09:24
@mj12albert mj12albert force-pushed the feat/collapsible-component branch 11 times, most recently from 0172ef8 to 939767f Compare August 15, 2024 11:09
@mj12albert mj12albert force-pushed the feat/collapsible-component branch from 939767f to 8296f06 Compare August 19, 2024 06:58
@mj12albert mj12albert marked this pull request as ready for review August 19, 2024 07:10
@mj12albert mj12albert force-pushed the feat/collapsible-component branch from 8296f06 to 244d479 Compare August 19, 2024 11:01
@mj12albert mj12albert force-pushed the feat/collapsible-component branch 3 times, most recently from a6676a5 to e83e63a Compare August 21, 2024 05:41
return renderElement();
});

export namespace CollapsibleContent {
Copy link
Member

Choose a reason for hiding this comment

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

👍

setHeight(rect.height);
}

element.style.animationName = shouldCancelAnimation ? 'none' : originalAnimationName;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we need to do all this on our side. Have you considered applying the data-instant attribute and instructing users to disable all motion when it's present? (perhaps this could also solve the issue with transitions)

Copy link
Member Author

@mj12albert mj12albert Aug 21, 2024

Choose a reason for hiding this comment

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

@michaldudak data-instant applies conditions that last for the lifetime of the component, which is different for the cases that are handled internally here, which only disables motion for one specific instance/occurrence of the open state changing:

  • When the component is initially open, on first mount/load, motion is disabled so it's just appears fully open, but animations run normally after that
  • When the component uses hidden="until-found" and is opened by in-page-search, it will open instantly, but animations run normally afterwards

I don't think these behaviors should require users to opt-in to it (or be override-able) for animated Collapsibles as it obviously results in poorer UX

Also this keeps the component easier to style, without having to add another animation state in addition to the 4 standard ones!

(I've also added a comment in the code about this here)

@mj12albert mj12albert force-pushed the feat/collapsible-component branch from 949ca4f to c5289c7 Compare August 22, 2024 09:52
@mj12albert mj12albert force-pushed the feat/collapsible-component branch from c5289c7 to 3e0f351 Compare August 23, 2024 13:47
@mj12albert
Copy link
Member Author

@colmtuite It's ready for you to have one last look before we merge it ~

@colmtuite colmtuite merged commit 8224de7 into mui:master Aug 28, 2024
18 checks passed
@mj12albert mj12albert deleted the feat/collapsible-component branch August 28, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: collapsible This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Collapsible] Implement Collapsible
5 participants