fix(useDialogContainer): defaultContainerProps #691
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 496a7d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
❌ Deploy Preview for cube-uikit-docs failed.
|
Contributor
📦 NPM canary releaseDeployed canary version 0.0.0-canary-52b5bf6. |
Contributor
🏗 Docs are successfully deployed!👀 Preview: https://68418c24677316d9e5f8ed2e--cube-uikit-docs.netlify.app |
Contributor
🧪 Storybook is successfully deployed!
|
Contributor
🏋️ Size limit report
Click here if you want to find out what is changed in this build |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying default props on the DialogContainer in the useDialogContainer hook and ensures they’re merged correctly.
- Introduce and document the
defaultContainerPropsparameter in the hook signature. - Change the spread of container props to
mergeProps(defaultContainerProps, containerProps)on theDialogContainer. - Remove inappropriate merging of
defaultContainerPropsinto the dialog content component and update effect dependencies to includedefaultContainerProps.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/components/overlays/Dialog/dialog-container.tsx | Add defaultContainerProps JSDoc, update merge logic for container props, adjust component props spread, and update hook dependencies |
| .changeset/afraid-mugs-sneeze.md | Add patch changeset entry describing the fix to defaultContainerProps handling |
Comments suppressed due to low confidence (3)
src/components/overlays/Dialog/dialog-container.tsx:18
- Specify in the JSDoc that
defaultContainerPropsis optional and, if possible, reference its generic type (e.g.,CP) to make the signature clearer for consumers.
* @param defaultContainerProps - Default props to pass to the DialogContainer.
src/components/overlays/Dialog/dialog-container.tsx:69
- Introduce unit tests covering scenarios where
defaultContainerPropsis provided alone, overridden bycontainerProps, and both combined, to ensure the merged output behaves as intended.
{...mergeProps(defaultContainerProps, containerProps || undefined)}
src/components/overlays/Dialog/dialog-container.tsx:69
- [nitpick] Since
mergePropsalready handlesundefined, you can simplify this call tomergeProps(defaultContainerProps, containerProps)and drop the|| undefinedfallback.
{...mergeProps(defaultContainerProps, containerProps || undefined)}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.