feat: Popover migration#1153
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
📖 Storybook Preview |
📖 Storybook Preview |
|
Socket Security detected that the npm publisher changed from ryan.roemer (a personal account) to formidablelabs (the org account). Socket flags all publisher changes because account takeovers are a real supply chain attack vector. Why it's safe to dismiss: The transfer is a legitimate housekeeping move within the same organization:
Recommended action: Add a @SocketSecurity ignore comment on the PR to dismiss the alert. @georgewrmarshall any thoughts on this? 👀 |
3982ddb to
b536e21
Compare
📖 Storybook Preview |
📖 Storybook Preview |
georgewrmarshall
left a comment
There was a problem hiding this comment.
Component Migration Review: Popover
Reviewed against:
- Component migration strategy
.cursor/rules/component-migration.md.cursor/rules/component-architecture.md.cursor/rules/styling.md
Summary
The Popover migration looks close. The implementation, stories, tests, and migration docs are in good shape overall, but I left a few follow-ups around the positioning dependency choice, migration architecture consistency, and styling conventions in the new files.
Key Findings
react-popperintroduces a React support mismatch against this package’s advertised React 19 peer range.- This migration currently lands as React-only with local types, which doesn’t fully match the documented migration workflow for shared types and cross-platform readiness.
- A couple of new files lean on static inline/CSS object styling where our styling guidance would prefer Tailwind classes or component props.
Follow-up
Separate from this PR, it’s probably worth discussing a move from react-popper to @floating-ui/react. Floating UI’s current React package supports react >=17 / react-dom >=17, while react-popper is in maintenance mode.
| } as const; | ||
| export type PopoverRole = (typeof PopoverRole)[keyof typeof PopoverRole]; | ||
|
|
||
| export type PopoverProps = Omit<ComponentProps<'div'>, 'role'> & { |
There was a problem hiding this comment.
question: this migration currently keeps Popover types local to design-system-react, and I’m not seeing the shared-type / React Native side of the migration. Our migration docs and ADR-0004 patterns usually expect migrated components to land with shared types and cross-platform readiness, even if alignment is partial. Is this intended as an explicit React-only exception, or should that follow-up be captured somewhere?
There was a problem hiding this comment.
@georgewrmarshall I think it's intentional, because we don't have Popover equivalent on mobile side? And if we try to move something into shared folder it can be harder to re-work the code later, because we will think that we had an API design and intentionally made this split...
a6ebd89 to
a029dc2
Compare
📖 Storybook Preview |
|
Could we make sure to include these changes from the extension version MetaMask/metamask-extension#42732 🙏 |
a029dc2 to
a25c8b2
Compare
📖 Storybook Preview |
📖 Storybook Preview |
There was a problem hiding this comment.
Strong migration work with comprehensive documentation. The implementation correctly uses Floating UI and maintains full prop parity with the extension version. Approving with a few non-blocking documentation clarifications needed to align with the actual implementation.
Can we add code examples similar to the extension storybook. Looks like we are missing some stories as well can be added as follow up if easier
popoer.mov
| "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" | ||
| }, | ||
| "dependencies": { | ||
| "@floating-ui/react-dom": "^2.1.2", |
There was a problem hiding this comment.
non-blocking: Good catch adding @floating-ui/react-dom. The MIGRATION.md and README.mdx files still reference react-popper in several places and should be updated to reflect that this implementation uses Floating UI, not Popper.js.
|
|
||
| ### Popover Component | ||
|
|
||
| The extension `popover` component maps to `Popover` in the design system. The runtime contract — `referenceElement`, `isOpen`, `position`, `role`, `hasArrow`, `matchWidth`, `flip`, `preventOverflow`, `referenceHidden`, `offset`, `isPortal`, `arrowProps`, `onPressEscKey`, `onClickOutside` — is preserved 1:1, including the `PopoverPosition` and `PopoverRole` value strings. The breaking changes are limited to the surrounding API surface: `react-popper` is now an internal runtime dependency (not a peer), the polymorphic `as` / Box style-utility passthrough is gone, and the legacy SCSS class hooks (`.mm-popover`, `.mm-popover__arrow`, `.mm-popover--reference-hidden`) are replaced by Tailwind utilities and inline styles. |
There was a problem hiding this comment.
non-blocking: This says react-popper is now an internal runtime dependency but the implementation uses @floating-ui/react-dom. All references to react-popper in this migration guide should be updated to @floating-ui/react-dom for accuracy.
| | `isOpen?: boolean` | `isOpen?: boolean` | unchanged | nothing renders when `false` | | ||
| | `position?: PopoverPosition` | `position?: PopoverPosition` | unchanged | same string values; default `PopoverPosition.Auto` still forces `flip` and `preventOverflow` on | | ||
| | `role?: PopoverRole` | `role?: PopoverRole` | unchanged | same string values; default `PopoverRole.Tooltip` | | ||
| | `hasArrow?: boolean` | `hasArrow?: boolean` | unchanged | default `false`; the rendered notch rotates with the resolved placement | |
There was a problem hiding this comment.
non-blocking: Same react-popper vs @floating-ui/react-dom discrepancy here. The positioning library column should say Floating UI, not Popper.js.
| | `matchWidth?: boolean` | `matchWidth?: boolean` | unchanged | matches the reference's `clientWidth` when `true` | | ||
| | `preventOverflow?: boolean` | `preventOverflow?: boolean` | unchanged | forced on when `position === PopoverPosition.Auto` | | ||
| | `flip?: boolean` | `flip?: boolean` | unchanged | forced on when `position === PopoverPosition.Auto` | | ||
| | `referenceHidden?: boolean` | `referenceHidden?: boolean` | unchanged | default `true`; behavior preserved (hides popover when `data-popper-reference-hidden="true"`) — implemented via Tailwind `data-[]:` variants instead of SCSS | |
There was a problem hiding this comment.
suggestion: Consider adding inline code examples or story links under each prop section in the README, similar to how the extension component-library documents props. This would improve discoverability for consumers migrating from the extension.
📖 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 d1b8b48. Configure here.
| className, | ||
| )} | ||
| {...props} | ||
| style={popoverStyle} |
There was a problem hiding this comment.
Modal dismisses portaled popover clicks
Medium Severity
The new Popover root never sets data-mm-modal-ignore-outside-click, while ModalContent ignores outside clicks only when that attribute is present on the click target. Legacy extension popovers carried the mm-popover hook for the same behavior. A portaled Popover opened inside a modal is treated as an outside click and closes the modal on interaction.
Reviewed by Cursor Bugbot for commit d1b8b48. Configure here.
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> Follow up for #1153 ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-313 ## **Manual testing steps** This is just PR with addressing comments from previous PR. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** N/A ### **After** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only edits to migration and README; no code or dependency changes in this PR. > > **Overview** > Updates **Popover** migration and Storybook docs so they describe **`@floating-ui/react-dom`** as the internal positioning stack instead of **`react-popper`**, matching the implementation from the prior PR. > > **`MIGRATION.md`** revises the Popover section: positioning library notes, `referenceElement` description, default/behavior table (including arrow rendering as a real `<Box>` child), and API differences. **`Popover/README.mdx`** updates the intro and references link accordingly. No runtime or API behavior changes in this diff. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3b7c784. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Release 43.0.0 This release drops Node.js 18 support across the release line, adds several new components, and includes a small set of breaking API changes that are documented in the migration guides. ### 📦 Package Versions - `@metamask/design-system-shared`: **0.21.0** - `@metamask/design-system-react`: **0.25.0** - `@metamask/design-system-react-native`: **0.28.0** - `@metamask/design-tokens`: **8.5.0** - `@metamask/design-system-tailwind-preset`: **0.9.0** - `@metamask/design-system-twrnc-preset`: **0.5.0** ### 🔄 Shared Type Updates (0.21.0) #### Added - Added `ContentPropsShared` and `ContentVerticalAlignment` for React Native list-style rows and related layout patterns ([#1192](#1192)) #### Changed - **BREAKING:** Dropped Node.js 18 support for the release line; consumers must run Node 20 or newer ([#1206](#1206)) - **BREAKING:** Updated `TextAreaPropsShared` to remove `inputElement` so React Native `TextArea` can render the root `TextInput` directly ([#1205](#1205)) ### 🌐 React Web Updates (0.25.0) #### Added - Added `Popover` for anchored overlays such as menus, tooltips, and dialogs ([#1153](#1153)) - Added `TextArea` for controlled multiline text entry ([#1036](#1036)) - Added `TextFieldSearch` for controlled search-field flows on top of `TextField` ([#1171](#1171)) - Added `FormTextField` for labeled form controls built from `Label`, `TextField`, and `HelpText` ([#1197](#1197)) #### Changed - **BREAKING:** Dropped Node.js 18 support for the release line; consumers must run Node 20 or newer ([#1206](#1206)) - Updated avatar fallback handling so `AvatarToken`, `AvatarNetwork`, and `AvatarFavicon` resolve consistently when the requested image is unavailable ([#1212](#1212)) ### 📱 React Native Updates (0.28.0) #### Added - Added `Content` for composing scrollable and padded content sections on React Native screens; it is closely related to the upcoming `ListItem` work ([#1192](#1192)) #### Changed - **BREAKING:** Dropped Node.js 18 support for the release line; consumers must run Node 20 or newer ([#1206](#1206)) - Added default padding and `isInteractive` support to `SectionHeader` so section rows match the new mobile layout patterns ([#1210](#1210)) - **BREAKING:** Flattened `TextArea` so it renders the root `TextInput` directly; pass `TextInput` props on `TextArea`, use the component `ref` for the input, and stop relying on `inputProps` or `inputElement` ([#1205](#1205)) - Updated avatar fallback handling so `AvatarToken`, `AvatarNetwork`, and `AvatarFavicon` resolve consistently when the requested image is unavailable ([#1212](#1212)) ###⚠️ Breaking Changes #### Node.js 18 support removed **What Changed:** - The release line now requires Node 20 or newer. - This applies across the monorepo, including the shared package, web package, React Native package, tokens, and both preset packages. **Impact:** - Consumers still on Node 18 must upgrade their runtime before installing or developing against this release line. - Node 18 is end-of-life, so this change aligns the repo with the supported app runtimes. #### React Native `TextArea` flattening **What Changed:** - `TextArea` now renders the root `TextInput` directly. - `inputProps` and `inputElement` are removed. - `inputRef` is replaced by the component `ref`. **Migration:** ```tsx // Before (0.27.0) <TextArea inputProps={{ placeholder: 'Message' }} inputElement={<CustomInput />} /> // After (0.28.0) <TextArea placeholder="Message" ref={inputRef} /> ``` **Impact:** - Affects React Native consumers using `TextArea`. - Call sites that depended on the wrapper/input split need to be updated. See migration guides for complete instructions: - [React Migration Guide](./packages/design-system-react/MIGRATION.md#from-version-0220-to-0230) - [React Native Migration Guide](./packages/design-system-react-native/MIGRATION.md#from-version-0270-to-0280) ### ✅ Checklist - [x] Changelogs updated with human-readable descriptions - [x] Changelog validation passed (`yarn changelog:validate`) - [x] Version bumps follow semantic versioning - [x] design-system-shared: minor (0.20.0 → 0.21.0) - shared type additions and breaking TextArea/shared runtime baseline - [x] design-system-react: minor (0.24.0 → 0.25.0) - new components and release-line update - [x] design-system-react-native: minor (0.27.0 → 0.28.0) - new component, SectionHeader update, and breaking TextArea change - [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** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) - [x] 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** - [x] I've reviewed the [Reviewing Release PRs](./docs/reviewing-release-prs.md) guide - [x] Package versions follow semantic versioning - [x] Changelog entries are consumer-facing (not commit message regurgitation) - [x] Breaking changes are documented in MIGRATION.md with examples - [x] All unreleased changes are accounted for in changelogs <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Breaking React Native TextArea and Node 18 removal affect consumer upgrades; most diff is release metadata with coordinated peer dependency bumps. > > **Overview** > **Release 43.0.0** bumps the monorepo root to **43.0.0** and publishes coordinated semver bumps across design-system packages, with **yarn.lock** peer ranges updated for `@metamask/design-system-tailwind-preset` **^0.9.0** and `@metamask/design-system-twrnc-preset` **^0.5.0**. > > Across the release line, changelogs record **Node.js 18 dropped** (Node **20+** required). **@metamask/design-system-react** **0.25.0** documents new **`Popover`**, **`TextArea`**, **`TextFieldSearch`**, and **`FormTextField`**, plus avatar fallback fixes. **@metamask/design-system-react-native** **0.28.0** adds **`Content`**, updates **`SectionHeader`** (default padding, **`isInteractive`**), and includes a **breaking** **`TextArea`** flattening (`inputProps` / `inputElement` / `inputRef` removed; props and **`ref`** target the root **`TextInput`**). **@metamask/design-system-shared** **0.21.0** adds **`ContentPropsShared`** / **`ContentVerticalAlignment`** and removes **`inputElement`** from shared **`TextArea`** props. > > Migration guide edits in this diff: React Native **0.27.0 → 0.28.0** **`TextArea`** guidance; React version heading **0.22.0 → 0.23.0** for **`BannerBase`** (changelog-driven **0.25.0** items are not new migration sections here). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 23b0cda. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->


Description
Added
Popovercomponent to DSR.Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-313
Manual testing steps
PopovercomponentScreenshots/Recordings
Before
Screen.Recording.2026-05-06.at.14.32.29.mov
After
Screen.Recording.2026-05-06.at.14.30.38.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Touches a widely reused overlay primitive with document-level listeners and positioning semantics; regressions could affect menus/tooltips across extension call sites, though behavior is heavily tested and documented.
Overview
Adds a
Popovercomponent to@metamask/design-system-reactfor anchored overlays (menus, tooltips, dialogs), with Storybook, Jest coverage, package exports, and a largeMIGRATION.mdsection for moving off the extensioncomponent-librarypopover.Positioning is implemented with
@floating-ui/react-dom(useFloating, flip/shift/autoPlacement, arrow, hide), while keeping the familiar prop contract (referenceElement,isOpen, placement, portal, match width, Esc / click-outside). The surface is a fixeddivwith tokenizedBoxstyling and Tailwind (rounded-lg,shadow-md, reference-hiddendata-*variants) instead of polymorphicas, Box utility passthrough, and.mm-popoverSCSS.PopoverPosition/PopoverRoleare const objects with string unions (ADR-0003); document listeners for Escape and outside click are registered and removed with matching{ capture: true }options.Extension adopters are guided toward import-path-only migrations where possible, with
className/arrowProps.classNamefor overrides. Note: some in-repo comments and migration text still mentionreact-popper, while the runtime dependency is Floating UI.Reviewed by Cursor Bugbot for commit d1b8b48. Bugbot is set up for automated code reviews on this repo. Configure here.