Skip to content

feat: Popover migration#1153

Merged
kirillzyusko merged 9 commits into
mainfrom
feat/popover-migration
Jun 4, 2026
Merged

feat: Popover migration#1153
kirillzyusko merged 9 commits into
mainfrom
feat/popover-migration

Conversation

@kirillzyusko

@kirillzyusko kirillzyusko commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Description

Added Popover component to DSR.

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/DSYS-313

Manual testing steps

  1. Open Storybook
  2. Check Popover component

Screenshots/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

  • I've followed MetaMask Contributor Docs
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

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 Popover component to @metamask/design-system-react for anchored overlays (menus, tooltips, dialogs), with Storybook, Jest coverage, package exports, and a large MIGRATION.md section for moving off the extension component-library popover.

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 fixed div with tokenized Box styling and Tailwind (rounded-lg, shadow-md, reference-hidden data-* variants) instead of polymorphic as, Box utility passthrough, and .mm-popover SCSS. PopoverPosition / PopoverRole are 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.className for overrides. Note: some in-repo comments and migration text still mention react-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.

@kirillzyusko kirillzyusko self-assigned this May 5, 2026
@socket-security

socket-security Bot commented May 5, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​floating-ui/​react-dom@​2.1.81001007385100

View full report

@socket-security

socket-security Bot commented May 5, 2026

Copy link
Copy Markdown

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.

View full report

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@kirillzyusko

Copy link
Copy Markdown
Collaborator Author

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:

  • The GitHub repo has always been at github.com/FormidableLabs/react-fast-compare — the org always owned the code
  • ryan.roemer is the co-founder of Formidable Labs, which was acquired by NearForm in Oct 2023
  • Moving the npm publish right from the founder's personal account to the org account is expected after an acquisition
  • react-popper is also a Formidable Labs project, so the same org controls both the dep and its consumer
  • 3.2.2 is the latest version (published ~3 years ago) — no new code was injected

Recommended action: Add a @SocketSecurity ignore comment on the PR to dismiss the alert.

@georgewrmarshall any thoughts on this? 👀

@kirillzyusko kirillzyusko force-pushed the feat/popover-migration branch from 3982ddb to b536e21 Compare May 6, 2026 12:11
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@kirillzyusko kirillzyusko marked this pull request as ready for review May 6, 2026 12:37
@kirillzyusko kirillzyusko requested a review from a team as a code owner May 6, 2026 12:38
Comment thread packages/design-system-react/src/components/Popover/Popover.tsx Outdated
Comment thread packages/design-system-react/src/components/Popover/Popover.types.ts Outdated
@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

Comment thread packages/design-system-react/package.json Outdated

@georgewrmarshall georgewrmarshall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-popper introduces 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.

Comment thread packages/design-system-react/package.json Outdated
} as const;
export type PopoverRole = (typeof PopoverRole)[keyof typeof PopoverRole];

export type PopoverProps = Omit<ComponentProps<'div'>, 'role'> & {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Comment thread packages/design-system-react/src/components/Popover/README.mdx Outdated
Comment thread packages/design-system-react/src/components/Popover/Popover.constants.ts Outdated
Comment thread packages/design-system-react/src/components/Popover/Popover.stories.tsx Outdated
@kirillzyusko kirillzyusko force-pushed the feat/popover-migration branch from a6ebd89 to a029dc2 Compare May 8, 2026 10:21
@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@georgewrmarshall

Copy link
Copy Markdown
Contributor

Could we make sure to include these changes from the extension version MetaMask/metamask-extension#42732 🙏

@kirillzyusko kirillzyusko force-pushed the feat/popover-migration branch from a029dc2 to a25c8b2 Compare June 2, 2026 10:06
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

Comment thread packages/design-system-react/src/components/Popover/Popover.tsx
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@georgewrmarshall georgewrmarshall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kirillzyusko kirillzyusko enabled auto-merge (squash) June 4, 2026 08:19
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d1b8b48. Configure here.

@kirillzyusko kirillzyusko merged commit 80cdc26 into main Jun 4, 2026
36 checks passed
@kirillzyusko kirillzyusko deleted the feat/popover-migration branch June 4, 2026 08:23
@kirillzyusko kirillzyusko mentioned this pull request Jun 4, 2026
7 tasks
georgewrmarshall pushed a commit that referenced this pull request Jun 4, 2026
<!--
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 -->
@georgewrmarshall georgewrmarshall mentioned this pull request Jun 4, 2026
18 tasks
georgewrmarshall added a commit that referenced this pull request Jun 4, 2026
## 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants