[2/N] feat: ModalFocus migration (extension)#1128
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📖 Storybook Preview |
📖 Storybook Preview |
📖 Storybook Preview |
ModalFocus migration (extension)ModalFocus migration (extension)
| import type { ModalFocusProps } from './ModalFocus.types'; | ||
| import README from './README.mdx'; | ||
|
|
||
| const meta: Meta<ModalFocusProps> = { |
There was a problem hiding this comment.
Stories look good!
georgewrmarshall
left a comment
There was a problem hiding this comment.
I compared this migration against the current extension modal-focus implementation and ran the focused ModalFocus test file on the PR snapshot. The wrapper behavior looks fine, but I found one package-contract issue in how react-focus-lock is being published, plus one non-blocking storybook follow-up.
| "react": "^17.0.0 || ^18.0.0 || ^19.0.0", | ||
| "react-dom": "^17.0.0 || ^18.0.0 || ^19.0.0" | ||
| "react-dom": "^17.0.0 || ^18.0.0 || ^19.0.0", | ||
| "react-focus-lock": "^2.9.4" |
There was a problem hiding this comment.
suggestion: I think react-focus-lock should live in dependencies, not peerDependencies. ModalFocus wraps it as an internal implementation detail, and if the migration goal is to remove the legacy extension component-library version, consumers should depend on @metamask/design-system-react rather than needing to know about the underlying focus-trap library too. Exposing react-focus-lock as a peer here leaks that implementation choice into the package contract and creates an avoidable install/runtime footgun. If we move it to dependencies, I think it can also drop out of devDependencies, and the migration docs can stay closer to an import-path swap instead of adding a new consumer-install step.
| <ModalFocus {...args}> | ||
| <FocusTrappedBox> | ||
| <Text>Modal focus children</Text> | ||
| <input aria-label="example input" /> |
There was a problem hiding this comment.
suggestion: Can we use the design-system Input component in these stories instead of raw <input> elements? Since these are the public usage examples for ModalFocus, it would be better to demonstrate focus trapping with DSR primitives where we can. If the longer-term direction is to show TextField once that API is ready, Input seems like the right intermediate step.
f21b2f1 to
d6cd616
Compare
📖 Storybook Preview |
📖 Storybook Preview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 e16b9df. Configure here.
| import React, { useRef, useState } from 'react'; | ||
|
|
||
| import { Box, BoxBorderColor, BoxFlexDirection } from '../Box'; | ||
| import { Button, ButtonVariant } from '../Button'; |
There was a problem hiding this comment.
ButtonVariant imported from sibling instead of shared package
Low Severity
ButtonVariant is imported from '../Button' instead of @metamask/design-system-shared. This violates the rule requiring migrated shared types/const objects to be imported from the shared package in stories. The sibling ModalOverlay.stories.tsx correctly uses import { ButtonVariant } from '@metamask/design-system-shared' alongside import { Button } from '../Button' — the component itself comes from the sibling, but the const object comes from shared.
Triggered by learned rule: Import shared types from @metamask/design-system-shared
Reviewed by Cursor Bugbot for commit e16b9df. Configure here.
📖 Storybook Preview |
## 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
ModalFocuscomponents to DSR package.Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-309
Manual testing steps
ModalFocuscomponentScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Introduces a new exported focus-trap component and adds
react-focus-lockas a runtime dependency; focus management changes can cause subtle accessibility or interaction regressions if behavior differs across bundlers/environments.Overview
Adds a new
ModalFocuscomponent to@metamask/design-system-react, wrappingreact-focus-lockand exposing the extension-compatible API (initialFocusRef,finalFocusRef,restoreFocus,autoFocus).Exports
ModalFocus(andFocusableElement/ModalFocusProps) from the package barrel, adds Storybook docs/stories plus unit tests, and updatesMIGRATION.mdwith an extension-to-DSR import-path migration section. Also makesreact-focus-lockan internal dependency of the design-system package.Reviewed by Cursor Bugbot for commit f22caf3. Bugbot is set up for automated code reviews on this repo. Configure here.