WB-1635: Button - Allow styling the inner label element #2112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 14eba89 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2112 +/- ##
==========================================
- Coverage 97.06% 97.06% -0.01%
==========================================
Files 243 243
Lines 28203 28186 -17
Branches 2462 2416 -46
==========================================
- Hits 27376 27359 -17
Misses 827 827
Continue to review full report in Codecov by Sentry.
|
|
Size Change: -63 B (0%) Total Size: 92.6 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (be8bc36) 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="PR2112"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2112 |
nishasy
approved these changes
Nov 15, 2023
| whiteSpace: "normal", | ||
| }, | ||
| }, | ||
| render: (args: any) => ( |
There was a problem hiding this comment.
Praise: Great time to use args!
| @@ -488,25 +482,12 @@ export const _generateStyles = ( | |||
| }, | |||
| }, | |||
| focus: { | |||
| ":after": { | |||
There was a problem hiding this comment.
Praise: I love that we can remove this!
There was a problem hiding this comment.
I think the test on line 124 is failing here because of the border/outline update. (It won't let me comment that line directly)
await expect(computedStyleButton.borderWidth).toBe("2px");
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ufchrqymlg.chromatic.com/ Chromatic results:
|
jandrade
added a commit
that referenced
this pull request
Nov 17, 2023
## Summary: In #2112, I introduced a new `labelStyle` prop and refactored the styles for the secondary button. However, I missed a case where the border was not being applied correctly, causing the button from being cut off in certain scenarios. This PR fixes that issue by including the `border` in the resting/default state, so the button is always rendered with a border (including that as part of the box model). NOTE: The issue was noticed while trying to integrate the changes in webapp. <img width="621" alt="Screenshot 2023-11-15 at 6 00 42 PM" src="https://github.com/Khan/wonder-blocks/assets/843075/32f76420-1aff-47c9-ad34-cb4aa8976bff"> Issue: WB-1635 ## Test plan: Verify that the secondary button is rendered correctly in all scenarios. This means that now secondary buttons will always have a border in the resting state. Author: jandrade Reviewers: jeresig Required Reviewers: Approved By: jeresig Checks: ✅ codecov/project, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Lint (ubuntu-latest, 16.x), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 16.x), ⏭ Chromatic - Skip on Release PR (changesets), ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ⏭ dependabot Pull Request URL: #2124
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The current button doesn’t allow to display the text contents in multiple lines.
This is required so we can remove the truncate functionality for certain cases
like the
ActionBubblecomponent in webapp. This is needed so we can wrap thetext instead and allow the button’s content to be displayed in two (or more)
lines when needed.
This PR adds a new prop
labelStyleto theButtoncomponent that allows tostyle the inner label element.
Also changed the
borderproperty tooutlineto be consistent with theoverall strategy for visual indicatorss in all the WB components. This will also
help us to avoid some layout issues in case folks override the button styles.
Issue: https://khanacademy.atlassian.net/browse/WB-1635
Test plan:
Verify that the following story works as expected:
/?path=/story/button--custom-styles
By working as expected, it means that the custom button should be displayed
in multiple lines and the text should be wrapped instead of being truncated.
Also verify that the other stories work as expected.