chore: update anvil modal size api#37033
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/client/packages/design-system/widgets/src/components/Modal/src/Modal.tsx (1)
22-23: Component props update looks goodThe
data-sizeattribute andoverlayPropsspread provide a cleaner API for modal customization.Consider adding a JSDoc comment above the Modal component to document the available size options and usage of overlayProps.
+/** + * Modal component with customizable size and overlay properties + * @param size - Modal size ("small" | "medium" | "large") + * @param overlayProps - Additional props to be spread on the overlay + */ export const Modal = (props: ModalProps) => {app/client/src/modules/ui-builder/ui/wds/WDSModalWidget/widget/index.tsx (1)
143-153: LGTM! Consider reordering props for better readability.The Modal API changes look good with the new
sizeprop andoverlayProps. For better readability, consider grouping related props together:<Modal isOpen={this.state.isVisible as boolean} onClose={this.onModalClose} + size={this.props.size} overlayProps={{ [AnvilDataAttributes.MODAL_SIZE]: this.props.size, [AnvilDataAttributes.WIDGET_NAME]: this.props.widgetName, [AnvilDataAttributes.IS_SELECTED_WIDGET]: this.props.isWidgetSelected ? "true" : "false", }} setOpen={(val) => this.setState({ isVisible: val })} - size={this.props.size} >app/client/packages/design-system/widgets/src/components/Modal/stories/ModalExamples.tsx (2)
127-127: LGTM! Consider adding size documentation.The size prop implementation is consistent. Consider adding a comment or updating component documentation to list available size options for future developers.
Line range hint
119-124: Consider documenting default size behavior.The medium modal doesn't specify a size prop, relying on the default value. Consider adding a comment to explicitly indicate that medium is the default size for better code clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/packages/design-system/widgets/src/components/Modal/src/Modal.tsx (2 hunks)
- app/client/packages/design-system/widgets/src/components/Modal/src/types.ts (2 hunks)
- app/client/packages/design-system/widgets/src/components/Modal/stories/ModalExamples.tsx (2 hunks)
- app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1 hunks)
- app/client/src/modules/ui-builder/ui/wds/WDSModalWidget/widget/index.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/client/packages/design-system/widgets/src/components/Modal/src/Modal.tsx (2)
1-6: Import reordering looks good
11-13: Props update looks good, verify migration impactThe new props structure with
overlayPropsandsizeis well-defined with appropriate defaults.Let's verify all Modal usages have been updated:
app/client/packages/design-system/widgets/src/components/Modal/src/types.ts (2)
6-6: LGTM: Clean type import
19-22: Verify: Is excluding 'xSmall' size intentional?The type definition explicitly omits the 'xSmall' size option. Please confirm if this is intentional, as it might affect existing modal implementations.
app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1)
208-208: LGTM! Modal size prop update aligns with the new API.The change from using dataAttributes to the new size prop follows the updated Modal API design.
Let's verify other Modal instances are updated consistently:
✅ Verification successful
Let me try a different approach to verify the Modal component usage.
This should help us:
- Find Modal component usage patterns
- Check if there are any remaining dataAttributes props
- Verify the consistency of size prop usage
The
-Uflag enables multiline matching, and we're including both .ts and .tsx files in the search.
Let me try one more time with just the file extension without type specification.
Based on the search results, I can see that the Modal component has been updated across the codebase to use a new API design that includes
sizeprop instead ofdataAttributes. The search shows no remaining usage ofdataAttributesprop with Modal components, which confirms the migration is complete.The size="small" prop update is correct and consistent with the new Modal API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining dataAttributes usage in Modal components rg -l "Modal.*dataAttributes" --type tsxLength of output: 70
Script:
#!/bin/bash # Search for Modal components with their props rg -U "Modal.*?>" --type ts --type tsx -A 2 # Also search for any remaining dataAttributes usage rg "dataAttributes" --type ts --type tsxLength of output: 142
Script:
#!/bin/bash # Find all TypeScript/React files first fd "\.(tsx|ts)$" # Then search for Modal usage in those files fd "\.(tsx|ts)$" -x rg -l "Modal.*dataAttributes" # And search for Modal components with their props fd "\.(tsx|ts)$" -x rg -U "Modal.*?>" -A 2Length of output: 391747
app/client/packages/design-system/widgets/src/components/Modal/stories/ModalExamples.tsx (1)
57-57: LGTM! Clean API improvement.The change from data attributes to a dedicated size prop improves the API's clarity and usability.
app/client/packages/design-system/widgets/src/components/Modal/src/types.ts
Show resolved
Hide resolved
| /** The children of the component. */ | ||
| children: ReactNode; | ||
| /** Additional props to be passed to the overlay */ | ||
| overlayProps?: Record<string, string>; |
There was a problem hiding this comment.
Besides the date attributes, there doesn't seem to be any point in pass anything else here, right? Maybe we should keep the prev name?
Fixes the API for modal for setting modal. /ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11479189189> > Commit: 1a07d20 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11479189189&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Wed, 23 Oct 2024 12:09:48 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `size` prop for the Modal component, allowing for flexible sizing options (e.g., small, medium, large). - Added `overlayProps` for enhanced customization of the Modal's overlay properties. - **Bug Fixes** - Removed the outdated `dataAttributes` prop, streamlining the Modal configuration. - **Documentation** - Updated examples to reflect the new prop structure for the Modal component. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Fixes the API for modal for setting modal.
/ok-to-test tags="@tag.Anvil"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11479189189
Commit: 1a07d20
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Wed, 23 Oct 2024 12:09:48 UTC
Summary by CodeRabbit
New Features
sizeprop for the Modal component, allowing for flexible sizing options (e.g., small, medium, large).overlayPropsfor enhanced customization of the Modal's overlay properties.Bug Fixes
dataAttributesprop, streamlining the Modal configuration.Documentation