Skip to content

fix(ui-buttons): fix ai-secondary button size diff#2617

Open
git-nandor wants to merge 1 commit into
masterfrom
INSTUI-5061_ai_button_size_fix
Open

fix(ui-buttons): fix ai-secondary button size diff#2617
git-nandor wants to merge 1 commit into
masterfrom
INSTUI-5061_ai_button_size_fix

Conversation

@git-nandor

@git-nandor git-nandor commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

INSTUI-5061

Summary

An ai-secondary button was 2px taller (and, for icon-only buttons, 2px wider) than the same button in any other color.

ai-secondary removes the real border and renders its gradient "border" as a ::before ring whose space is reserved by padding. That padding is additive to the content's full minHeight, adding 2px to the outer size.

The fix shrinks the ai-secondary content box by 2 × borderWidth (height, plus width for icon-only buttons), so the reserved padding is compensated and the outer size is identical regardless of color.

Update (review feedback)

  • Condensed sizes also covered.
  • Refactor. Collapsed the duplicated calc / size list / height tokens into a single aiSecondaryContentSize const.
  • Regression coverage. The button page now renders every size's ai-secondary button next to a default one so the screenshot catches any size drift, condensed included.

Test:

Verify that the buttons have the same size.

{[
  'small',
  'medium',
  'large',
  'condensedSmall',
  'condensedMedium'
].map((size) => (
  <div
    key={size}
  >
    <h3>{size}</h3>

    {/* text: ai-secondary vs default */}
    <Button size={size} color="ai-secondary">{size}</Button>
    <Button size={size} color="secondary">{size}</Button>

    {/* icon-only: ai-secondary vs default (width parity) */}
    <IconButton size={size} color="ai-secondary" screenReaderLabel="Add">
      <IconAddLine />
    </IconButton>
    <IconButton size={size} color="secondary" screenReaderLabel="Add">
      <IconAddLine />
    </IconButton>

    {/* icon-only circle ai-secondary */}
    <IconButton size={size} shape="circle" color="ai-secondary" screenReaderLabel="Add">
      <IconAddLine />
    </IconButton>
    <IconButton size={size} shape="circle" color="secondary" screenReaderLabel="Add">
      <IconAddLine />
    </IconButton>
  </div>
))}

@git-nandor git-nandor self-assigned this Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://instructure.design/pr-preview/pr-2617/

Built to branch gh-pages at 2026-07-17 09:41 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Visual regression report

⚠️ Changes detected.

Status Count
Unchanged 31
Changed 1
New 0
Removed 0

📊 View full report

Diff images (1)

button-and-derivatives.png — 474230 pixels differ

Baselines come from the visual-baselines branch. They refresh on every merge to master.

github-actions Bot pushed a commit that referenced this pull request Jul 1, 2026
@git-nandor
git-nandor marked this pull request as ready for review July 1, 2026 13:17
@balzss

balzss commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Nice fix, root cause is spot on and I checked that small/medium/large are pixel-perfect now (both icon and text buttons).

Condensed sizes are still off. The fix only kicks in for small/medium/large, but the wrapper padding at line 477 gets added for every size — so condensedSmall and condensedMedium ai-secondary buttons are still 2px taller and wider. Confirmed by measuring. Since condensed is a real combo we need to cover it too — heads up, condensed uses height instead of minHeight, so it needs its own little branch subtracting 2 × borderWidth there.

Add a case to the regression test suite — render the ai-secondary buttons next to the default ones (all sizes) on the button page so the screenshot catches any size drift.

Small cleanup on the code itself. Right now the calc is written twice (minHeight + width), the size list is repeated (once as the heightForAiSize keys, once in the size === ... check), and heightForAiSize duplicates height tokens we already have. You can collapse all three — pull the value into one const and index the theme directly:

const aiContentSize =
  color === 'ai-secondary' && componentTheme[`${size}Height`]
    ? `calc(${componentTheme[`${size}Height`]} - ${componentTheme.borderWidth} * 2)`
    : undefined

then use aiContentSize for both minHeight and the icon-only width. No map, no repeated calc, no repeated size list. (You'll still want the separate condensed branch since those don't have a ${size}Height token.)

Heads up: this only works because it's spread last and overrides the minHeight from sizeVariants — that's the existing pattern here, just good to know.

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

see my (and claude's 😺 ) comment

@git-nandor
git-nandor force-pushed the INSTUI-5061_ai_button_size_fix branch from e0ff957 to 0fc74bd Compare July 17, 2026 08:28
github-actions Bot pushed a commit that referenced this pull request Jul 17, 2026
@git-nandor

Copy link
Copy Markdown
Contributor Author

Thanks, the fix and the cleanup is done.

@git-nandor
git-nandor force-pushed the INSTUI-5061_ai_button_size_fix branch from 0fc74bd to 826e59f Compare July 17, 2026 09:37
@git-nandor
git-nandor requested review from balzss and joyenjoyer July 17, 2026 09:38
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