feat: ModalFooter migration (extension)#1132
Conversation
📖 Storybook Preview |
📖 Storybook Preview |
📖 Storybook Preview |
9bbf2dc to
d068b0b
Compare
📖 Storybook Preview |
georgewrmarshall
left a comment
There was a problem hiding this comment.
I compared this against the existing React Native BottomSheetFooter API as well as the extension modal-footer migration target. The migration direction looks reasonable, but I think we should align the new public footer API across platforms before this settles: the current web ModalFooter introduces a different action model (onSubmit / onCancel + button prop bags) and a fixed layout shape where RN already has a cleaner primaryButtonProps / secondaryButtonProps + buttonsAlignment contract.
| * Optional click handler for the submit button. When provided, a primary | ||
| * "submit" button is rendered. | ||
| */ | ||
| onSubmit?: () => void; |
There was a problem hiding this comment.
suggestion: Can we align this public API with React Native BottomSheetFooter before we ship a second footer contract? RN already uses primaryButtonProps / secondaryButtonProps and buttonsAlignment, with the component owning ordering and variants: https://github.com/MetaMask/metamask-design-system/blob/main/packages/design-system-react-native/src/components/BottomSheetFooter/BottomSheetFooter.types.ts. This web version currently splits responsibility between top-level onSubmit / onCancel and nested button prop bags, which makes the cross-platform model harder to share in docs/tests and leaves more room for divergence. Since this component is new, I think it would be cleaner to adopt the same shape here and let button presence be driven by primaryButtonProps / secondaryButtonProps.
| containerProps?.className, | ||
| )} | ||
| > | ||
| {onCancel && ( |
There was a problem hiding this comment.
suggestion: If we align this with RN BottomSheetFooter, I think the component should own the button semantics the same way the native implementation does: render secondary/primary from secondaryButtonProps / primaryButtonProps, enforce ButtonVariant.Secondary / ButtonVariant.Primary internally, and make layout explicit through a buttonsAlignment prop instead of hardcoding the current wrapped-horizontal arrangement. RN already does that here: https://github.com/MetaMask/metamask-design-system/blob/main/packages/design-system-react-native/src/components/BottomSheetFooter/BottomSheetFooter.tsx. That would also avoid exposing variant / onClick as something consumers can use to partially bypass the footer contract.
| * className and will be overridden if `containerProps.className` provides | ||
| * its own `max-w-*` utility (`twMerge` lets the consumer's class win). | ||
| */ | ||
| containerProps?: Omit<BoxProps, 'children'> & { |
There was a problem hiding this comment.
suggestion: Can we exclude asChild from containerProps here? BoxProps currently leaks asChild, but this footer’s internal Box can render two buttons. If a consumer passes containerProps={{ asChild: true }}, Box switches to Radix Slot, which only supports a single child element, so this public type surface can typecheck and then fail at runtime. Omit<BoxProps, 'children' | 'asChild'> seems closer to the intended contract.
d068b0b to
472daad
Compare
📖 Storybook Preview |
| export type { | ||
| ModalFooterButtonProps, | ||
| ModalFooterProps, | ||
| } from './ModalFooter.types'; |
There was a problem hiding this comment.
Nested prop bag type exported from component entrypoint
Low Severity
ModalFooterButtonProps is exported from the component entrypoint. This is an internal nested prop bag type (the shape of primaryButtonProps/secondaryButtonProps) and expands the public API surface unnecessarily. Consumers can derive it via ModalFooterProps['primaryButtonProps'] instead.
Triggered by learned rule: Don't export internal nested prop bag types from component entrypoints
Reviewed by Cursor Bugbot for commit 472daad. Configure here.
📖 Storybook Preview |
📖 Storybook Preview |
georgewrmarshall
left a comment
There was a problem hiding this comment.
I rechecked the current head against the earlier review round. The footer API is now aligned with the React Native BottomSheetFooter direction: button presence is driven by primaryButtonProps / secondaryButtonProps, buttonsAlignment is part of the public contract, the component owns button variants, and the old containerProps surface is gone. I also ran the focused ModalFooter test file on the current head and it passes. I don’t have any remaining blocking findings on this round.
e5f7507 to
12ed5e4
Compare
📖 Storybook Preview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 12ed5e4. Configure here.
| export const ButtonsAlignment = { | ||
| Horizontal: 'horizontal', | ||
| Vertical: 'vertical', | ||
| } as const; |
There was a problem hiding this comment.
ButtonsAlignment values mismatch with React Native counterpart
Medium Severity
The ButtonsAlignment const values are 'horizontal' / 'vertical' (lowercase), but the React Native BottomSheetFooter it claims to mirror uses 'Horizontal' / 'Vertical' (PascalCase). The JSDoc says "Mirrors BottomSheetFooter's ButtonsAlignment" and the migration guide says the API is "reshaped to align with the React Native BottomSheetFooter," but the underlying string values don't actually match. This will force a breaking change on one platform when ButtonsAlignment moves to design-system-shared.
Reviewed by Cursor Bugbot for commit 12ed5e4. Configure here.
## Release 38.0.0 This release updates the shared, web, native, tokens, and Tailwind packages with new cross-platform input and avatar-group contracts, new modal building blocks for React, a breaking React Native Toast API redesign, and Tailwind CSS v4 support for web consumers. ### 📦 Package Versions - `@metamask/design-system-shared`: **0.16.0** - `@metamask/design-system-react`: **0.21.0** - `@metamask/design-system-react-native`: **0.23.0** - `@metamask/design-system-tailwind-preset`: **0.7.0** - `@metamask/design-tokens`: **8.4.0** ### 🔄 Shared Type Updates (0.16.0) #### Input and AvatarGroup shared contracts ([#1043](#1043), [#1067](#1067)) **What Changed:** - Added shared `Input` contracts for controlled `value`, `isReadOnly`, and `isStateStylesDisabled` - Added shared `AvatarGroup` size, variant, and prop contracts - Added the shared `Merge` icon export ([#1155](#1155)) **Impact:** - React and React Native consumers can build against one aligned input and avatar-group API surface - Cross-platform wrappers can depend on `@metamask/design-system-shared` instead of maintaining platform-specific type assumptions ### 🌐 React Web Updates (0.21.0) #### Added - Added `ModalOverlay`, `ModalBody`, `ModalFocus`, and `ModalFooter` to support Extension modal migrations into `@metamask/design-system-react` ([#1120](#1120), [#1121](#1121), [#1128](#1128), [#1132](#1132)) - Added the `Merge` icon to the React icon set ([#1155](#1155)) #### Changed - Updated `Input` to follow the shared controlled input API and use `isReadOnly` as the public readonly prop name ([#1043](#1043)) - Updated `AvatarGroup` to use shared cross-platform size and variant contracts ([#1067](#1067)) ### 📱 React Native Updates (0.23.0) #### Added - Added the `Merge` icon to the React Native icon set ([#1155](#1155)) #### Changed - **BREAKING:** Redesigned `Toast` to use a single mounted `<Toast />` plus static `Toast.show(...)` and `Toast.hide()` methods for application usage ([#1104](#1104)) - Removed `ToastContext`, `ToastContextWrapper`, and `ToastContextParams` from the public export surface - Renamed `ToastVariants` to `ToastVariant`, changed icon-only close buttons to `ToastCloseButtonVariant.Icon`, and renamed `customBottomOffset` to `bottomOffset` - `Toast.show()` and `Toast.hide()` now throw a descriptive error if called before `<Toast />` mounts - See the [React Native Migration Guide](./packages/design-system-react-native/MIGRATION.md#from-version-0220-to-0230) - Updated `Input` to follow the shared controlled input API and rename `isReadonly` to `isReadOnly` ([#1043](#1043)) - Updated `AvatarGroup` to use shared cross-platform size and variant contracts ([#1067](#1067)) ### 🎨 Tokens and Tailwind Updates #### `@metamask/design-tokens` 8.4.0 - Added `@metamask/design-tokens/tailwind/theme.css` for Tailwind CSS v4 consumers, providing a single import for token variables, theme values, typography, fonts, and shadow utilities ([#1117](#1117)) #### `@metamask/design-system-tailwind-preset` 0.7.0 - Added a `fade-in` animation utility so consumers can use `animate-fade-in` for simple opacity entrance transitions, including the new `ModalOverlay` web migration path ([#1120](#1120)) - Clarified that Tailwind CSS v3 consumers should keep using this preset, while Tailwind CSS v4 consumers should migrate to `@metamask/design-tokens/tailwind/theme.css` ([#1117](#1117)) ###⚠️ Breaking Changes #### Toast API redesign (React Native) **What Changed:** - `Toast` application usage moved from context/service patterns to static `Toast.show(...)` and `Toast.hide()` methods - `ToastContext`, `ToastContextWrapper`, and `ToastContextParams` were removed from the public API - `ToastVariants` was renamed to `ToastVariant` - Icon-only close buttons now use `ToastCloseButtonVariant.Icon` - `customBottomOffset` was renamed to `bottomOffset` **Impact:** - Existing `@metamask/design-system-react-native` consumers using the old Toast context flow need import, root-mount, and call-site updates - Existing app code must ensure `<Toast />` is mounted before invoking `Toast.show()` / `Toast.hide()` See migration guides for complete instructions: - [React Native Migration Guide](./packages/design-system-react-native/MIGRATION.md#from-version-0220-to-0230) ### ✅ Checklist - [x] Changelogs updated with human-readable descriptions - [x] Changelog validation passed (`yarn workspace <package> changelog:validate`) - [x] Version bumps follow semantic versioning - [x] design-system-shared: minor (`0.15.0` → `0.16.0`) - new shared `Input`, `AvatarGroup`, and icon exports - [x] design-system-react: minor (`0.20.0` → `0.21.0`) - new modal components, icon, and shared API alignment - [x] design-system-react-native: minor (`0.22.0` → `0.23.0`) - breaking Toast redesign, icon, and shared API alignment - [x] design-system-tailwind-preset: minor (`0.6.1` → `0.7.0`) - new `fade-in` utility and Tailwind CSS v4 migration guidance - [x] design-tokens: minor (`8.3.0` → `8.4.0`) - Tailwind CSS v4 `theme.css` export - [x] Breaking changes documented with migration guidance - [x] Migration guides updated with before/after examples (if breaking changes) - [x] PR references included in changelog entries ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) - [ ] I've reviewed the [Release Workflow](./.cursor/rules/release-workflow.md) cursor rule - [ ] All tests pass (`yarn build && yarn test && yarn lint`) - [x] Changelog validation passes (`yarn changelog:validate`) ## **Pre-merge reviewer checklist** - [ ] I've reviewed the [Reviewing Release PRs](./docs/reviewing-release-prs.md) guide - [ ] Package versions follow semantic versioning - [ ] Changelog entries are consumer-facing (not commit message regurgitation) - [ ] Breaking changes are documented in MIGRATION.md with examples - [ ] All unreleased changes are accounted for in changelogs
Description
Added
ModalFootermigration to DSR.Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-308
Manual testing steps
ModalFooterspecScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Low risk: changes are additive (new
ModalFootercomponent, Storybook docs/stories, and tests) plus migration documentation updates, with no modifications to existing component behavior.Overview
Adds a new
ModalFootercomponent to@metamask/design-system-react, rendering an optional primary/secondary action button row with a newbuttonsAlignmentoption and enforcing button variants internally.Exports
ModalFooter/ButtonsAlignmentfrom the package barrel, and includes full Storybook docs/stories plus unit tests. UpdatesMIGRATION.mdwith extension-to-design-system breaking-change mappings and before/after examples forModalFooter.Reviewed by Cursor Bugbot for commit 12ed5e4. Bugbot is set up for automated code reviews on this repo. Configure here.