Skip to content

Conversation

@langermank
Copy link
Contributor

  • Adds background-color for the default Button disabled state
  • Extends disabled Storybook stories for better snapshot tests (will be more useful when this PR merges)

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@langermank langermank requested a review from a team as a code owner March 12, 2024 00:47
@langermank langermank requested a review from jonrohan March 12, 2024 00:47
@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2024

🦋 Changeset detected

Latest commit: 2516a21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@langermank langermank added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 12, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 87.57 KB (-0.03% 🔽)
packages/react/dist/browser.umd.js 87.96 KB (+0.15% 🔺)

@github-actions github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Mar 12, 2024
.c0:disabled [data-component=ButtonCounter] {
color: inherit;
background-color: var(--button-default-bgColor-disabled,undefined);
Copy link
Member

Choose a reason for hiding this comment

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

This seems weird, but noticing the rest of the snapshot has similar undefined 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh.. that's not good. Investigating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this looks like a normal thing that happens. I tested it in Storybook and the value is there. Its not great but we're getting rid of snaps soon anyways. Thanks for flagging!

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a very similar comment. Ignore me if this is expected behavior.

'&:disabled': {
color: 'primer.fg.disabled',
borderColor: `var(--button-default-borderColor-disabled, ${theme?.colors.btn.border})`,
backgroundColor: `var(--button-default-bgColor-disabled, ${theme?.colors.input.disabledBg})`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure theme.colors.input.disabledBg is the right path for the fallback? It's rendering as undefined in the Jest snapshot tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the border color above is also undefined, so I assumed there's something up with the test

@langermank langermank added this pull request to the merge queue Mar 12, 2024
Merged via the queue into main with commit cb54f42 Mar 12, 2024
@langermank langermank deleted the fix-disabled-states branch March 12, 2024 20:43
@primer primer bot mentioned this pull request Mar 12, 2024
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
* btn bg + stories

* Create young-cooks-shave.md

* test(vrt): update snapshots

* snippy snappers

* move bg color

---------

Co-authored-by: langermank <[email protected]>
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.

4 participants