Skip to content

feat: add override icon to Notice#1010

Draft
Peechiz wants to merge 2 commits into
mainfrom
feat/notice-override-icon
Draft

feat: add override icon to Notice#1010
Peechiz wants to merge 2 commits into
mainfrom
feat/notice-override-icon

Conversation

@Peechiz
Copy link
Copy Markdown
Contributor

@Peechiz Peechiz commented May 21, 2026

Closes n/a
see: CORE-177

✅ Pull Request Checklist

  • Included link to corresponding GitHub Issue.
  • The commit message follows conventional commit extended guidelines.
  • Added/updated unit tests and storybook for this change (for bug fixes / features).
  • Added/updated visual regression tests for this change (for bug fixes / features).
  • Added/updated documentation (for bug fixes / features)
  • Filled out test instructions.
  • Added changeset (for bug fixes / features).

📝 Test Instructions

Allows passing a custom icon into the <Notice overrideIcon={<MyIcon />} />

The automatic color-based switching is useful for basic notices, but shouldn't disallow customization for client apps.

❓ Does this PR introduce a breaking change?

  • Yes
  • No

🤖 AI Usage

  • Added corresponding label (ai / human) to PR:

If ai was used, select all that apply:

  • Ideation / brainstorming
  • Documentation
  • Testing
  • Implementation

💬 Other information

@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
design-toolkit Ready Ready Preview, Comment May 21, 2026 6:08pm
map-toolkit Ready Ready Preview, Comment May 21, 2026 6:08pm

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

🧠 Memory Leak Test Results

Status: ✅ All tests passed

Component Leaks Retained Size Status
accordion-group 0 0 B
accordion 0 0 B
actionbar 0 0 B
avatar 5 110.72 KB
badge 0 0 B
button 0 0 B
dialog 0 0 B
drawer 0 0 B
floating-card 0 0 B
floatingcard 0 0 B
intentional-leak 0 0 B
notice 0 0 B
tooltip 2 44.27 KB
📋 Test Details
  • Components tested: 13
  • Total leaks detected: 7
  • Workflow run: View details

🤖 Generated by MemLab + Playwright

Comment on lines +84 to +85
const showOverrideIcon = iconOverride && size === 'medium';
const showIcon = !hideIcon && size === 'medium' && !iconOverride;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure why we pass down size to <NoticeIcon /> since it only ever renders at medium size. It's further confusing to see the size={size === 'small' ? 'medium' : 'large'} inside the NoticeIcon, since presumably this component never gets a value other than medium

Copy link
Copy Markdown
Contributor

@switzerb switzerb May 21, 2026

Choose a reason for hiding this comment

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

There is a note about this in the ticket -- I believe it is just a bug (or AI hallucination) that missed the QA gate.

There is an additional bug here where we only render the icon if the Notice size is medium, but in the Notice component we are checking to see if the size is small and then rendering a medium size icon – which makes no sense. The component will only render if the size is medium, so we can remove that conditional and just render the correct size of icon.

@github-actions
Copy link
Copy Markdown
Contributor

📊 Coverage Reports

Coverage Changes by Package

Click to expand 29 package details

apps/next (No diff)

packages/bus (No diff)

packages/constants (No diff)

packages/converters (No diff)

packages/core (No diff)

packages/dataset (No diff)

packages/design-foundation (No diff)

packages/design-toolkit

metric current base change
lines 86.43% 86.42% 0.01
statements 86.43% 86.42% 0.01
functions 83.26% 83.26% 0.00
branches 78.78% 78.81% -0.03

packages/formatters (No diff)

packages/geo (No diff)

packages/hotkey-manager (No diff)

packages/icons (No diff)

packages/logger (No diff)

packages/map-toolkit (No diff)

packages/math (No diff)

packages/ntds (No diff)

packages/postcss-tailwind-css-modules (No diff)

packages/predicates (No diff)

packages/temporal (No diff)

packages/web-worker (No diff)

packages/websocket (No diff)

tooling/biome-config (No diff)

tooling/constellation-tracker (No diff)

tooling/eslint-config (No diff)

tooling/prettier-config (No diff)

tooling/smeegl (No diff)

tooling/turbo-filter (No diff)

tooling/typescript-config (No diff)

tooling/vitest-config (No diff)

Coverage data collected from all packages in the monorepo.

@Peechiz Peechiz changed the title feat: add override icon to notice feat: add override icon to Notice May 21, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🖼️ Visual Regression Test Results

Status: ✅ All tests passed

Metric Count
✅ Passed 2726
❌ Failed 0
Total 2726

Component Coverage

45 / 54 design-toolkit components have VRT tests (83%)

Missing VRT tests (9 components) - audio - carousel - deferred-collection - floating-card - gantt - lines - media-controls - status-indicator - video
> 4 components excluded: hotkey, icon, skeleton, view-stack --- 🤖 Generated by Vitest Browser + Playwright

@switzerb
Copy link
Copy Markdown
Contributor

Yay! Thank you for picking up work. Here's the ticket: https://gitlab.accelint.dev/core-ux/standard-toolkit/-/issues/92

I think the solution you have in place may not be sufficient because mostly those Notice components are generated, and you won't be able to pass a function (React component) through the event bus. It has to be serializable.

@Peechiz Peechiz marked this pull request as draft May 21, 2026 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human AI was not used at all in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants