diff --git a/docs/evals/ui-development-reshape-2026-11-05-rerun.md b/docs/evals/ui-development-reshape-2026-11-05-rerun.md new file mode 100644 index 00000000..e108a31c --- /dev/null +++ b/docs/evals/ui-development-reshape-2026-11-05-rerun.md @@ -0,0 +1,220 @@ +# UI development skills reshape eval, re-run after ISS fixes — 2026-11-05 + +This re-runs the [first eval](ui-development-reshape-2026-11-05.md) with the same method, after the actionable ISS fixes were applied to both PRs. + +PRs: [#212](https://github.com/MetaMask/skills/pull/212) (Mobile), [#213](https://github.com/MetaMask/skills/pull/213) (Extension) + +Skill heads under test: + +- #212 `cursor/mmds-mobile-skills-refresh-7309` @ `02bb1c2` (first run: `2d1ba46`) +- #213 `cursor/mmds-extension-skills-no-storybook-mcp-7309` @ `a46f691` (first run: `9c57432`) +- A/B baseline: `main` @ `c40ef31` (unchanged) + +The fixes are one shared commit, byte-identical on both branches (`6868dbc` on #212, `1ae0f98` on #213), plus one overlay commit per platform. Neither PR edits `CHANGELOG.md` in these commits. + +Consumers (sparse shallow clones of `main`, 2026-11-05): + +| Consumer | Commit | MMDS package (lockfile) | Shared | Preset | +| --- | --- | --- | --- | --- | +| metamask-mobile | `178a32b` | `design-system-react-native` 0.55.0 | `design-system-shared` 0.44.0 | `design-system-twrnc-preset` 0.14.0 | +| metamask-extension | `79ab2fd` | `design-system-react` **0.51.0** (was 0.47.0) | `design-system-shared` 0.45.0 | `design-system-tailwind-preset` 0.16.0 | + +Extension `main` bumped its MMDS packages in [metamask-extension#47305](https://github.com/MetaMask/metamask-extension/pull/47305) (merged 21:51 UTC), after the first eval. As in the first run, installed types come from the npm tarballs at the lockfile versions (ISS-001). Storybook MCP and Context Forge were treated as unavailable. + +## Summary + +| Scenario | Mobile (#212) | Extension (#213) | First run | +| --- | --- | --- | --- | +| 1. Types win over manifest | Pass | Pass | Pass / Pass | +| 2. Restyle with foundations | Pass | **Pass** | Pass / Partial | +| 3. Legacy* migration | Pass | **Pass** | Pass / Partial | +| 4. Review checklist | Pass | Pass | Pass / Pass | +| Install smoke | Pass | Pass | Pass / Pass (with ISS-002) | + +Both Extension Partials are now Pass. No new issues were found. + +Merge verdict: **still safe to merge together, still not separately** (ISS-003 is unchanged by design). Merging #212 then #213, or #213 then #212, onto `main` is conflict-free, and the shared files are identical on both branches. + +## Issue status + +| Issue | Severity | Status | Change | +| --- | --- | --- | --- | +| ISS-001 | note | Open (method) | Unchanged. Tarballs instead of a full `yarn install`. | +| ISS-002 | minor | **Fixed** | The base's Strategy link is now an absolute GitHub URL (returns 200). The duplicate Extension overlay link is removed. No `../knowledge` links remain in either installed copy. | +| ISS-003 | major | Open (merge order) | Unchanged. Merge #212 and #213 back to back. | +| ISS-004 | minor | **Fixed** | `reading-mmds-manifests.md` lists the non-component entry kinds (`design-tokens-`/`docs-`, `patterns-`, cast-expression names). It says to treat an entry as a component only when its `path` is under the platform package's `src/components/` and the name is in the installed export index. The Mobile overlay names `app/components/UI/MarketingMomentTitle`. | +| ISS-005 | minor | **Fixed** | Base: "Use the replacement the JSDoc names if the installed platform package exports it. If no replacement is named, or the named one is not exported on this platform, keep existing usage and record the gap." | +| ISS-006 | minor | **Fixed** | Extension: "Do not add SASS or SCSS. Deleting SCSS rules, files, and `@import` lines that your change makes unused is expected." The checklist matches. | +| ISS-007 | note | **Fixed** | Mobile: a bullet on retiring `useStyles` + `*.styles.ts`. The `ListItem` bullet now says `isInteractive` applies the pressed background itself. | +| ISS-008 | minor | **Fixed** | Extension: prefer `Box` typed props for layout and `className` for the rest. Follow accessible web practices for anything clickable: a native ` + +); +``` + +- `hover:bg-hover` and `active:bg-pressed` are preset-generated (`design-system-tailwind-preset/dist/colors.mjs:16-17`). There are no arbitrary values, and Tailwind removes the default palette and font sizes (`tailwind.config.js:23-30`). +- The cleanup deletes `funding-method-modal/index.scss` and its `@import` in `multichain-components.scss:27`. This literally "modifies SCSS" (ISS-006). +- Semantics improved: a native `button` replaces the clickable `div`. The overlay has no clickable-row pattern (ISS-008). + +### Scenario 3 — Legacy* migration + +**Mobile (#212) — Pass** (with note ISS-012; Notion status ISS-010) + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ✅ | ✅ | + +Target: `app/components/UI/Notification/SwitchLoadingModal/Loader.tsx`, which imports legacy default `Text` + `TextVariant` from `component-library/components/Texts/Text`, legacy default `Icon` + `IconSize`/`IconName`/`IconColor`, and MMDS `Button`. The incremental step migrates `Text` only: + +```tsx +import { + Button, + ButtonSize, + ButtonVariant, + Text, + TextVariant, +} from '@metamask/design-system-react-native'; +import LegacyIcon, { + IconColor as LegacyIconColor, + IconName as LegacyIconName, + IconSize as LegacyIconSize, +} from '../../../../component-library/components/Icons/Icon'; +// … + + {errorContent} + +``` + +- `TextVariant.BodyMD` becomes `TextVariant.BodyMd` (`design-system-shared/dist/types/Text/Text.types.d.cts:47`). The hand-set `lineHeight: 20` is dropped because the variant sets it (base: "Never hand-set … line heights"). +- The overlay's default-export example (`import LegacyText, { TextVariant as LegacyTextVariant } …`) maps directly onto `Icon`. Aliasing `Icon` here follows the base wording literally. Repository practice would leave it alone until MMDS `Icon` is imported (ISS-012). +- Finishing the step later: swap `LegacyIcon` for MMDS `Icon` (`IconColor.Error` → `IconColor.ErrorDefault`) and remove the `Legacy*` import. +- Notion: the `Legacy*` rule is **Proposed** (2026-11-04) while the skill enforces it as Active (ISS-010). + +**Extension (#213) — Partial** (ISS-009) + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ⚠️ overlay misses the enum path | ✅ | + +Target: `funding-method-item.tsx` (above), incremental step migrating `Text` only: + +```tsx +import { Icon, IconName, IconSize, Text, TextColor, TextVariant } from '@metamask/design-system-react'; +import { Box, Text as LegacyText } from '../../component-library'; +import { + AlignItems, + Display, + FlexDirection, + TextColor as LegacyTextColor, + TextVariant as LegacyTextVariant, +} from '../../../helpers/constants/design-system'; +``` + +- Legacy `Box` keeps its plain name because MMDS `Box` is not imported in this step (ISS-012 reading). `LegacyText` is needed only if some `Text` usages stay legacy. +- The enum aliases (`TextVariant`, `TextColor`) are the real clash, and they come from `helpers/constants/design-system`, a path the overlay never mentions (ISS-009). `Display`/`FlexDirection`/`AlignItems` are legacy-only and keep their names. +- Notion: same Proposed status as Mobile (ISS-010). + +### Scenario 4 — Review checklist + +**Mobile (#212) — Pass** + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ✅ | ✅ | + +Invented patch, a new `app/components/UI/Ramp/components/FeeSheet/FeeSheet.tsx`: + +```tsx +1 import React from 'react'; +2 import { ScrollView, StyleSheet, View } from 'react-native'; +3 import { +4 BottomSheet, +5 ButtonBase, +6 ButtonIcon, +7 ButtonPrimary, +8 IconName, +9 Text, +10 TextVariant, +11 } from '@metamask/design-system-react-native'; +12 +13 const styles = StyleSheet.create({ row: { padding: 15, borderRadius: 16 } }); +14 +15 export const FeeSheet = ({ onClose, onConfirm, fee }: Props) => ( +16 +17 undefined} /> +18 +19 +20 {fee} +21 +22 +23 ({ opacity: pressed ? 0.5 : 1 })} onPress={onConfirm}>Back +24 Confirm +25 +26 ); +``` + +Review findings, following the base Review workflow and the Mobile checklist: + +| # | File:line | Finding | Evidence | Fix | +| --- | --- | --- | --- | --- | +| 1 | `FeeSheet.tsx:7,24` | `ButtonPrimary` is not exported by the installed package (invented or future API). | `dist/components/index.d.cts` at 0.55.0 has no `ButtonPrimary`; it ships in 0.56.0. | `