Conversation
📖 Storybook Preview |
📖 Storybook Preview |
|
|
||
| figma.connect( | ||
| Tag, | ||
| 'https://www.figma.com/design/1D6tnzXqWgnUC3spaAOELN/%F0%9F%A6%8A-MMDS-Components?node-id=12339-6553', |
There was a problem hiding this comment.
Added this so there is better connection between code:design. It'll help with maintainability in the long term. I can remove it if this is premature at this stage of our DS.
If/when this PR is approved, I'll need to add the code link in Figma.
📖 Storybook Preview |
📖 Storybook Preview |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 55a1aac. Configure here.
| export { BannerAlert, BannerAlertSeverity } from './BannerAlert'; | ||
| export type { BannerAlertProps } from './BannerAlert'; | ||
|
|
||
| export { Tag, TagVariant, type TagProps } from './Tag'; |
There was a problem hiding this comment.
Export pattern inconsistent with rest of file
Low Severity
The Tag export uses inline type syntax (export { Tag, TagVariant, type TagProps }) in a single statement, while every other component in this file (25+ components) uses separate export and export type statements. This is the only instance of the inline pattern in the entire file.
Reviewed by Cursor Bugbot for commit 55a1aac. Configure here.
| Warning: 'warning', | ||
| Info: 'info', | ||
| } as const; | ||
| export type TagVariant = (typeof TagVariant)[keyof typeof TagVariant]; |
There was a problem hiding this comment.
This should be in the shared package (you can get Cursor to help you align these props with cursor rules
| flexDirection: BoxFlexDirection.Row, | ||
| alignItems: BoxAlignItems.Center, | ||
| twClassName: 'rounded-md self-start gap-0.5', | ||
| } as const; |
There was a problem hiding this comment.
these styling should not be stored as consts. It should just be used inline
| /** | ||
| * Visual emphasis (background and default text color for label / plain-text children). | ||
| */ | ||
| variant?: TagVariant; |
There was a problem hiding this comment.
A lot of these props can be extracted to shared (you can get cursor to do this)
| /** | ||
| * Visual emphasis (background and default text color for label / plain-text children). | ||
| */ | ||
| variant?: TagVariant; |
There was a problem hiding this comment.
Additionally I think this prop should be named either severity or semantic cc @georgewrmarshall
| /** | ||
| * Optional prop for a custom element to show at the start of the tag when no start icon is shown. | ||
| */ | ||
| startAccessory?: React.ReactNode; |
There was a problem hiding this comment.
label, start Accessory, and endAccessory can be reused with BoxRow
| const horizontalPaddingTw = [ | ||
| finalStartIconName ? 'pl-1' : 'pl-1.5', | ||
| finalEndIconName ? 'pr-1' : 'pr-1.5', | ||
| ].join(' '); |
There was a problem hiding this comment.
don't store these consts. just use inline
brianacnguyen
left a comment
brianacnguyen
left a comment
There was a problem hiding this comment.
I suggest to run each individual file through cursor to align with cursor rules
|
@georgewrmarshall @amandaye0h should this component also includes a prop called includesSemanticIcon? |
There was a problem hiding this comment.
Hi Amanda, great first component PR 🎉 this is a well structured component and it's clear you've put real thought into the API design! The render helper pattern, variant color mapping, and accessory/icon separation are all clean. There are a few architectural requirements from our cursor rules and some alignment gaps with the Figma design to address before this is ready to merge. I've walked through each one with full context and included Cursor/Claude prompts where they can help. Nothing here is a fundamental problem — just refinements to get it fully aligned with our standards.
| TextColor, | ||
| } from '../../types'; | ||
|
|
||
| export const TagVariant = { |
There was a problem hiding this comment.
Per ADR-0004, this const assertion should live in @metamask/design-system-shared/src/types/Tag/ rather than the React Native package. This matters because when a React (web) Tag component is built later, both platforms need to share the same const object — if it's defined in the platform package there's a risk they drift out of sync.
The mapping constants below (MAP_TAG_VARIANT_BACKGROUND, etc.) are fine to keep here since they reference React Native-specific tokens.
You can prompt Cursor/Claude to handle the migration:
Following .cursor/rules/component-architecture.md ADR-0004 pattern, move TagVariant
from Tag.constants.ts into packages/design-system-shared/src/types/Tag/Tag.types.ts,
create a TagPropsShared type there, update Tag.types.ts to import and extend it,
and update index.ts to export TagVariant directly from @metamask/design-system-shared.
Reference BadgeStatus as the golden path example.
| }; | ||
|
|
||
| /** Vertical padding (Tailwind `py-*`) */ | ||
| export const TAG_PADDING_VERTICAL_TW = 'py-0'; |
There was a problem hiding this comment.
TAG_PADDING_VERTICAL_TW = 'py-0' has no effect — py-0 is the Tailwind default, so this is a no-op. Remove it.
TAG_LAYOUT below is used in exactly one place. Single-use constants like this add indirection without benefit — a future engineer has to jump between files to understand the layout. Move these values inline into Tag.tsx directly.
| /** | ||
| * Visual emphasis (background and default text color for label / plain-text children). | ||
| */ | ||
| variant?: TagVariant; |
There was a problem hiding this comment.
Curious on your thought here, I'm leaning towards two things here:
-
Renaminh
varianttoseverity.BannerAlertandIconAlertuseseverityfor similar semantic set of states (neutral/info/success/error/warning). Keeping the prop name consistent across components makes the design system more predictable for consumers. -
The Figma component currently has
primarywhere the code hasInfo— there is noinfoin Figma and noprimaryin the code. I'd recommend keepingInfo(to stay consistent withBannerAlert) and updating the Figma component, but we could discuss before making this change
| /** | ||
| * Label string; when set, shown instead of string/number `children`. Matches Figma `Label` in Code Connect. | ||
| */ | ||
| label?: string; |
There was a problem hiding this comment.
suggestion: I realize label was in the original insight report but I think we can temove the label prop and textProps prop. BoxRow (which Tag will be used alongside) uses a TextOrChildren pattern — children is rendered as a Text component automatically when passed a string/number, and rendered as-is when passed a node. Tag should follow the same pattern. Having both label and children serve the same purpose creates a confusing API where two props compete.
In Tag.tsx and Tag.types.ts, remove the label and textProps props.
Update the component to render string/number children using the TextOrChildren
pattern from BoxRow — wrap string/number children in a Text component with
the severity's textColor, TextVariant.BodyXs, and FontWeight.Medium.
Node children should render as-is.
| const meta: Meta<typeof Tag> = { | ||
| title: 'Components/Tag', | ||
| component: Tag, | ||
| }; |
There was a problem hiding this comment.
Per our component documentation standards, the first story must always be Default with minimal args and all controls wired up via argTypes. This powers the Storybook controls panel.
const meta: Meta<typeof Tag> = {
title: 'Components/Tag',
component: Tag,
argTypes: {
severity: {
control: 'select',
options: Object.values(TagSeverity),
},
startIconName: { control: 'select', options: Object.values(IconName) },
endIconName: { control: 'select', options: Object.values(IconName) },
},
};
export const Default: Story = {
args: {
children: 'Tag',
severity: TagSeverity.Neutral,
},
};non-blocking: TagVariant is imported from './Tag.constants' — once moved to shared, import from '.' (the index) to match the pattern used across the codebase.
| 'https://www.figma.com/design/1D6tnzXqWgnUC3spaAOELN/%F0%9F%A6%8A-MMDS-Components?node-id=12339-6553', | ||
| { | ||
| props: { | ||
| variant: figma.enum('variant', { |
There was a problem hiding this comment.
The Figma property is mistmatched type not variant. This mapping will silently produce no output in Dev Mode. Based on what the outcome is for variant/severity update to:
severity: figma.enum('severity', {
neutral: TagSeverity.Neutral,
info: TagSeverity.Info,
success: TagSeverity.Success,
error: TagSeverity.Error,
warning: TagSeverity.Warning,
})Hold off on publishing (yarn figma:connect:publish) until the severity/variant prop rename and info/primary Figma alignment are resolved. Once those are confirmed, run yarn figma:connect:publish:dry-run first to validate.
| export { BannerAlert, BannerAlertSeverity } from './BannerAlert'; | ||
| export type { BannerAlertProps } from './BannerAlert'; | ||
|
|
||
| export { Tag, TagVariant, type TagProps } from './Tag'; |
There was a problem hiding this comment.
non-blocking: Every other component in this file uses separate export and export type statements. The inline type pattern here is the only instance of this style in the file:
// current
export { Tag, TagVariant, type TagProps } from './Tag';
// consistent with rest of file
export { Tag, TagSeverity } from './Tag';
export type { TagProps } from './Tag';Note: once TagSeverity is moved to shared, this export will change anyway — tackle after the shared types migration.
3 participants
Description
What is the goal of this PR?
ListItems,Sections, andKeyValuesWhat are the key updates?
Tag.figma.tsx), stories, tests, and README/Figma link docsRelated issues
JIRA: https://consensyssoftware.atlassian.net/browse/DSYS-568
Manual testing steps
yarn install>yarn build>yarn storybook:iosto see changesScreenshots/Recordings
Before
Component did not exist before this.
After
Screen.Recording.2026-04-08.at.7.47.53.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Low risk: this is an additive UI component with storybook/docs/tests, plus a new export; it doesn’t modify existing business logic or data handling.
Overview
Introduces a new
Tagcomponent todesign-system-react-native, including variant styling (background/text/icon color), optional start/end icons or accessories, and automatic padding adjustments based on icon presence.Adds supporting package surface area and tooling: exports via
components/index.ts, Storybook stories (and auto-generatedstorybook.requires.jsentry), Code Connect mapping inTag.figma.tsx, README documentation, and a new test suite covering rendering, styling, and icon/accessory behavior.Reviewed by Cursor Bugbot for commit 55a1aac. Bugbot is set up for automated code reviews on this repo. Configure here.