-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pill updates - updated colors, small pills are bigger for a11y #2154
Conversation
🦋 Changeset detectedLatest commit: 31ccbee The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
Size Change: +248 B (0%) Total Size: 92.1 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2154 +/- ##
==========================================
- Coverage 97.06% 95.55% -1.52%
==========================================
Files 244 245 +1
Lines 28256 28334 +78
Branches 2461 2344 -117
==========================================
- Hits 27428 27074 -354
- Misses 828 1260 +432
... and 48 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-qdcoscnlxs.chromatic.com/ Chromatic results:
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (5683aef) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2154" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2154 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! thanks for working on improving this! 🚢 🚀
I only have an open question about 20px vs 24px that I'd like to understand better before landing, but everything looks really solid to me.
// Font size should be at least 14px for accessibility. | ||
expect(pill).toHaveStyle({ | ||
fontSize: 14, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Thanks for adding this assertion
test.each` | ||
kind | color | ||
${"neutral"} | ${tokens.color.offBlack8} | ||
${"accent"} | ${tokens.color.blue} | ||
${"info"} | ${tokens.color.fadedBlue16} | ||
${"success"} | ${tokens.color.fadedGreen16} | ||
${"warning"} | ${tokens.color.fadedGold16} | ||
${"critical"} | ${tokens.color.fadedRed16} | ||
`( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: nice!
paddingRight: tokens.spacing.xSmall_8, | ||
borderRadius: tokens.spacing.xxSmall_6, | ||
// Minimum tap area recommendation for a11y | ||
height: tokens.spacing.large_24, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I remember that one of the changes was to make this 20px
tall. So if I understand correctly, we are using 24 instead for the reasons you stated in the comment right? If that's the case, it would be worth checking how that looks in the places where we are hard-coding 20px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that was originally the case, but then we were asked to just bump it up to 24 in this thread:
https://khanacademy.slack.com/archives/C8Z9DSKC7/p1693925664214919?thread_ts=1693923761.646739&cid=C8Z9DSKC7
They're being hard coded to 20px in webapp right now, so I'll just update the ones that will look okay at 24px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, that makes sense, thanks!
Summary:
Some updates need to be made to pill so they're more usable
within webapp.
More context for some of these changes can be found in
the following slack thread:
https://khanacademy.slack.com/archives/C8Z9DSKC7/p1693923761646739
Issue: https://khanacademy.atlassian.net/browse/WB-1602
Test plan:
yarn jest packages/wonder-blocks-pill/src/components/__tests__/pill.test.tsx
Storybook
/?path=/story/pill--variants
/?path=/story/pill--vertically-stacked