-
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
[WB-1851.1] Dropdown: Remove light
prop on dropdowns
#2430
Conversation
…nder-blocks-dropdown. This includes removing that in `SingleSelect` and `MultiSelect`.
🦋 Changeset detectedLatest commit: 8de5ab8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
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 (d8f5b03) 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="PR2430" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2430 |
Size Change: -365 B (-0.37%) Total Size: 98 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-afcrdkowql.chromatic.com/ Chromatic results:
|
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! 🎉 Getting rid of the light
prop simplifies the different state combinations we support!
<KindVariants light={false} /> | ||
<KindVariants light={true} /> | ||
<View style={[styles.gridRow]}> | ||
<LabelMedium>Default</LabelMedium> |
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.
suggestion (non-blocking): We can use LabeledField in the examples since it's available in main now! (I forgot to update the dropdown AllVariants stories 😅)
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.
Good call! I'll update this.
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! 🚀
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
? "currentColor" | ||
: tokens.color.white | ||
: disabled | ||
const iconColor = disabled |
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.
Nice to see this simplified!
}; | ||
} | ||
}, | ||
press: activePressedStyling, |
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: The other stateful style names follow a different naming convention, e.g. "disabled" or "active" as adjectives. Is press
a new convention or should it follow the rest (and the previous naming)?
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.
Good question! this new press
change is to follow a convention that we agreed with Design. I missed changing the variable name to make it more consistent, but I'll do it in a follow up PR. Stay tuned!
## Summary: Following up on #2430. This PR removes the `light` variant from the remaining form fields: - `LabeledTextField` - `TextField` - `TextArea` - `LabeledField` - `SearchField` This is based on the discussion with the design team where we decided to remove the light prop from Wonder Blocks due to its low usage and to prepare for the upcoming design changes. Issue: WB-1851 ## Test plan: Verify that the light prop is removed from the components mentioned in the summary. Author: jandrade Reviewers: beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ⏭️ dependabot, ✅ gerald Pull Request URL: #2437
Summary:
Removes the
light
prop from wonder-blocks-dropdown. This includes removingthat in
SingleSelect
andMultiSelect
.This is based on the discussion with the design team where we decided to
remove the
light
prop from Wonder Blocks due to its low usage and to preparefor the upcoming design changes.
Issue: https://khanacademy.atlassian.net/browse/WB-1851
Test plan:
Verify that the
light
prop is removed fromSingleSelect
andMultiSelect
.