Skip to content

feat: [DSRN] Added HeaderAlert#1064

Closed
brianacnguyen wants to merge 5 commits intomainfrom
dsrn/headeralert
Closed

feat: [DSRN] Added HeaderAlert#1064
brianacnguyen wants to merge 5 commits intomainfrom
dsrn/headeralert

Conversation

@brianacnguyen
Copy link
Copy Markdown
Contributor

@brianacnguyen brianacnguyen commented Apr 10, 2026

Description

This pull request adds a new React Native HeaderAlert component in @metamask/design-system-react-native. The header centers an IconAlert at IconSize.Lg on top of HeaderBase, matching the horizontal padding pattern used by HeaderStandard (px-2).

Related issues

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

Manual testing steps

  1. From the repo root, run React Native Storybook (e.g. yarn storybook:ios or yarn storybook:android per your setup).
  2. Open Components → HeaderAlert and confirm Default and Severity stories render the centered alert icon and expected colors for each severity.
  3. Interact with back/close where configured in stories (if you add handlers that log or toggle state) and confirm taps behave as expected.

Screenshots/Recordings

Before

N/A — new component.

After

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2026-04-09.at.22.32.48.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 (HeaderAlert unit tests added)
  • I’ve documented my code using JSDoc format if applicable (README + shared type JSDoc where added)
  • 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

Low Risk
Low risk: adds a new UI component plus Storybook coverage and unit tests, with minimal changes to existing exports and no sensitive logic.

Overview
Adds a new HeaderAlert component to @metamask/design-system-react-native that composes HeaderStandard and renders a centered IconAlert at IconSize.Lg, exposing severity and forwarding the remaining header action props.

Exports the new component/types from the React Native package, adds a shared HeaderAlertPropsShared type to design-system-shared, and wires up Storybook stories plus unit tests to verify severity-to-color mapping and prop forwarding.

Reviewed by Cursor Bugbot for commit 2dbdf2b. Bugbot is set up for automated code reviews on this repo. Configure here.

@brianacnguyen brianacnguyen self-assigned this Apr 10, 2026
@brianacnguyen brianacnguyen requested a review from a team as a code owner April 10, 2026 05:36
@github-actions
Copy link
Copy Markdown
Contributor

📖 Storybook Preview

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

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 03287fd. Configure here.

Comment thread packages/design-system-react-native/src/components/HeaderAlert/HeaderAlert.tsx Outdated
@github-actions
Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@github-actions
Copy link
Copy Markdown
Contributor

📖 Storybook Preview

Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Curious what the composition would be for this component and how it will be used. CurrentlyHeaderAlert omits children, which means if you want to show a title with the alert you have to put it as a Text component outside the header, as a sibling. The header and the title visually read as a unit, but in code they're disconnected. Should we be passing the title as children?

// Without children support — title is disconnected from the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error} />
<Text variant={TextVariant.HeadingSm}>Critical alert heading</Text>

// With children support — title belongs to the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error}>
  Critical alert heading
</HeaderAlert>

It might be helpful to see this component in the context of how it will be used e.g. BottomSheetAlert (old screenshot but UI should be similar to this)

Screenshot 2026-04-14 at 10 20 56 AM

@brianacnguyen
Copy link
Copy Markdown
Contributor Author

Curious what the composition would be for this component and how it will be used. CurrentlyHeaderAlert omits children, which means if you want to show a title with the alert you have to put it as a Text component outside the header, as a sibling. The header and the title visually read as a unit, but in code they're disconnected. Should we be passing the title as children?

// Without children support — title is disconnected from the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error} />
<Text variant={TextVariant.HeadingSm}>Critical alert heading</Text>

// With children support — title belongs to the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error}>
  Critical alert heading
</HeaderAlert>

It might be helpful to see this component in the context of how it will be used e.g. BottomSheetAlert (old screenshot but UI should be similar to this)

Screenshot 2026-04-14 at 10 20 56 AM

Curious what the composition would be for this component and how it will be used. CurrentlyHeaderAlert omits children, which means if you want to show a title with the alert you have to put it as a Text component outside the header, as a sibling. The header and the title visually read as a unit, but in code they're disconnected. Should we be passing the title as children?

// Without children support — title is disconnected from the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error} />
<Text variant={TextVariant.HeadingSm}>Critical alert heading</Text>

// With children support — title belongs to the header
<HeaderAlert onClose={() => {}} severity={IconAlertSeverity.Error}>
  Critical alert heading
</HeaderAlert>

It might be helpful to see this component in the context of how it will be used e.g. BottomSheetAlert (old screenshot but UI should be similar to this)

Screenshot 2026-04-14 at 10 20 56 AM

@georgewrmarshall according to the design on the left the title will be outside of the header
image

@github-actions
Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@georgewrmarshall
Copy link
Copy Markdown
Contributor

georgewrmarshall commented Apr 15, 2026

@brianacnguyen I hear you on the Figma spec showing them as separate rows — but I did an audit of the existing implementations in both extension and mobile, and both treat the alert icon + title as a single compositional unit, not disconnected siblings.

Extension (alert-modal.tsx#L119-L156)

AlertHeader renders icon + title together as one block:

function AlertHeader({ selectedAlert, customTitle }) {
  return (
    <Box gap={3} alignItems={AlignItems.center} textAlign={TextAlign.Center}>
      <Icon name={...} size={IconSize.Xl} color={...} />
      <Text variant={TextVariant.headingSm} marginTop={3} marginBottom={4}>
        {customTitle ?? reason ?? t('alert')}
      </Text>
    </Box>
  );
}

Then in the modal (line ~440):

<ModalHeader onClose={handleClose} startAccessory={...} />
<AlertHeader selectedAlert={selectedAlert} customTitle={customTitle} />

The close/back buttons are a separate ModalHeader row. The icon and title live together in AlertHeader.

Mobile (alert-modal.tsx#L33-L61)

Same pattern — Header renders icon + title as a single fragment:

const Header = ({ selectedAlert, iconColor, styles, headerAccessory }) => (
  <>
    {headerAccessory ?? (
      <View style={styles.iconWrapper}>
        <Icon name={...} size={IconSize.Xl} color={iconColor} />
      </View>
    )}
    <View style={styles.headerContainer}>
      <Text style={styles.headerText} variant={TextVariant.BodyMDBold}>
        {selectedAlert.title ?? strings('alert_system.alert_modal.title')}
      </Text>
    </View>
  </>
);

The problem with the current PR

The current HeaderAlert wraps HeaderStandard (navigation buttons) and renders only the IconAlert as children — but omits children from the type, so the title must be a disconnected sibling:

// Current — title disconnected from header
<HeaderAlert severity={...} onClose={...} />
<Text variant={TextVariant.HeadingSm}>Critical alert heading</Text>

But both platforms compose icon + title as one unit:

// Extension/Mobile — icon + title together
<NavigationRow onClose={...} />
<AlertHeader>
  <Icon ... />
  <Text>Critical alert heading</Text>
</AlertHeader>

What I think should change

The current implementation sits in a middle ground that doesn't match either platform pattern. I think HeaderAlert should go one of two directions:

Option A: Full header — navigation + icon + title. HeaderAlert extends HeaderStandard and accepts children for the title text, owning the complete header block.

<HeaderAlert severity={IconAlertSeverity.Error} onClose={...}>
  Critical alert heading
</HeaderAlert>

Option B: Presentational only — icon + title, no navigation. HeaderAlert is a reusable visual pairing that works consistently on pages, bottom sheets, or modals. Navigation is composed separately.

<HeaderStandard onClose={...} />
<HeaderAlert severity={IconAlertSeverity.Error}>
  Critical alert heading
</HeaderAlert>

Either way, the icon + title must live together — the current approach of icon-only inside the header with title outside doesn't match how either platform uses this pattern.

Or at least that's what I would suggest. In the current HeaderAlert we are combining navigation and icon which seems like the wrong combination to me.

Happy to align on whichever direction and merge. We can also file an issue to revisit once BottomSheetAlert is implemented if we want to see the composition in context first.

@brianacnguyen
Copy link
Copy Markdown
Contributor Author

@brianacnguyen I hear you on the Figma spec showing them as separate rows — but I did an audit of the existing implementations in both extension and mobile, and both treat the alert icon + title as a single compositional unit, not disconnected siblings.

Extension (alert-modal.tsx#L119-L156)

AlertHeader renders icon + title together as one block:

function AlertHeader({ selectedAlert, customTitle }) {
  return (
    <Box gap={3} alignItems={AlignItems.center} textAlign={TextAlign.Center}>
      <Icon name={...} size={IconSize.Xl} color={...} />
      <Text variant={TextVariant.headingSm} marginTop={3} marginBottom={4}>
        {customTitle ?? reason ?? t('alert')}
      </Text>
    </Box>
  );
}

Then in the modal (line ~440):

<ModalHeader onClose={handleClose} startAccessory={...} />
<AlertHeader selectedAlert={selectedAlert} customTitle={customTitle} />

The close/back buttons are a separate ModalHeader row. The icon and title live together in AlertHeader.

Mobile (alert-modal.tsx#L33-L61)

Same pattern — Header renders icon + title as a single fragment:

const Header = ({ selectedAlert, iconColor, styles, headerAccessory }) => (
  <>
    {headerAccessory ?? (
      <View style={styles.iconWrapper}>
        <Icon name={...} size={IconSize.Xl} color={iconColor} />
      </View>
    )}
    <View style={styles.headerContainer}>
      <Text style={styles.headerText} variant={TextVariant.BodyMDBold}>
        {selectedAlert.title ?? strings('alert_system.alert_modal.title')}
      </Text>
    </View>
  </>
);

The problem with the current PR

The current HeaderAlert wraps HeaderStandard (navigation buttons) and renders only the IconAlert as children — but omits children from the type, so the title must be a disconnected sibling:

// Current — title disconnected from header
<HeaderAlert severity={...} onClose={...} />
<Text variant={TextVariant.HeadingSm}>Critical alert heading</Text>

But both platforms compose icon + title as one unit:

// Extension/Mobile — icon + title together
<NavigationRow onClose={...} />
<AlertHeader>
  <Icon ... />
  <Text>Critical alert heading</Text>
</AlertHeader>

What I think should change

The current implementation sits in a middle ground that doesn't match either platform pattern. I think HeaderAlert should go one of two directions:

Option A: Full header — navigation + icon + title. HeaderAlert extends HeaderStandard and accepts children for the title text, owning the complete header block.

<HeaderAlert severity={IconAlertSeverity.Error} onClose={...}>
  Critical alert heading
</HeaderAlert>

Option B: Presentational only — icon + title, no navigation. HeaderAlert is a reusable visual pairing that works consistently on pages, bottom sheets, or modals. Navigation is composed separately.

<HeaderStandard onClose={...} />
<HeaderAlert severity={IconAlertSeverity.Error}>
  Critical alert heading
</HeaderAlert>

Either way, the icon + title must live together — the current approach of icon-only inside the header with title outside doesn't match how either platform uses this pattern.

Or at least that's what I would suggest. In the current HeaderAlert we are combining navigation and icon which seems like the wrong combination to me.

Happy to align on whichever direction and merge. We can also file an issue to revisit once BottomSheetAlert is implemented if we want to see the composition in context first.

@amandaye0h what do you think? Including the title in the header component as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants