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] Make parameters in useCollapsilbe* hooks mandatory #752

Closed
wants to merge 15 commits into from
Closed

[Collapsible] Make parameters in useCollapsilbe* hooks mandatory #752

wants to merge 15 commits into from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Oct 22, 2024 •
edited
Loading

@sai6855 sai6855 marked this pull request as draft October 22, 2024 07:44
@mui-bot
Copy link

mui-bot commented Oct 22, 2024 •
edited
Loading

Netlify deploy preview

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

Generated by 🚫 dangerJS against 1482bbc

@sai6855 sai6855 added enhancement This is not a bug, nor a new feature component: collapsible This is the name of the generic UI component, not the React module! and removed enhancement This is not a bug, nor a new feature labels Oct 22, 2024
sai6855
sai6855 commented Oct 22, 2024
View reviewed changes
packages/mui-base/src/Collapsible/Root/CollapsibleRoot.tsx Outdated
@@ -22,11 +22,11 @@ const CollapsibleRoot = React.forwardRef(function CollapsibleRoot(
forwardedRef: React.ForwardedRef<HTMLDivElement>,
) {
const {
animated,
animated = true,
Copy link
Contributor Author

@sai6855 sai6855 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added deafult props in component,to make it consistent with other components.

base-ui/packages/mui-base/src/Dialog/Root/DialogRoot.tsx

Line 25 in d2e58dd

animated = true,

base-ui/packages/mui-base/src/Menu/Root/MenuRoot.tsx

Line 20 in d2e58dd

animated = true,

@sai6855 sai6855 marked this pull request as ready for review October 22, 2024 08:04
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 29, 2024
@michaldudak
Copy link
Member

The default values are set in the hook. We can make it consistent and set everything on the component, though. In this case, I'd recommend making the hook parameters mandatory.

@mj12albert
Copy link
Member

mj12albert commented Nov 25, 2024 •
edited
Loading

We can make it consistent and set everything on the component, though. In this case, I'd recommend making the hook parameters mandatory.

@sai6855 Thanks for working on this 🙏 we discussed this and decided to generally assign default values to optional props in the component, and have hook params be required: #856

@michaldudak
Copy link
Member

@sai6855, if you could make the parameters in the hook mandatory, we can merge this PR.

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 28, 2024 •
edited
Loading

@sai6855, if you could make the parameters in the hook mandatory, we can merge this PR.

Got it, I'll make the changes. Some how i missed this notification

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 30, 2024
@sai6855 sai6855 changed the title [Collapsible] Add missing defaults to Collapsible* components [Collapsible] Make parameters useCollapsilbe* hooks manditory Dec 2, 2024
@sai6855 sai6855 changed the title [Collapsible] Make parameters useCollapsilbe* hooks manditory [Collapsible] Make parameters in useCollapsilbe* hooks mandatory Dec 2, 2024
@sai6855
Copy link
Contributor Author

sai6855 commented Dec 2, 2024

@michaldudak @mj12albert PR is ready for review

@mj12albert
Copy link
Member

@sai6855 Apologies, I folded all the changes into #889
Thanks for spending the time 🙇

@mj12albert mj12albert closed this Dec 3, 2024
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

@mj12albert mj12albert Awaiting requested review from mj12albert

Assignees
No one assigned
Labels
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.
4 participants
@sai6855 @mui-bot @michaldudak @mj12albert