Components: Add next Button, ButtonGroup #29230
Merged
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
sarayourfriend
added
[Package] Components
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
@sarayourfriend Oh wow! You took on a big one 😍 |
ItsJonQ
force-pushed
the
add/hooks-button
branch
from
20c3c9b to
0ec6606
Compare
|
I fixed a merge/rebase issue with the |
|
Looks like there are some TS building issues with 🙈 |
|
@ItsJonQ we just need to update the |
sarayourfriend
changed the title
Thank you @sarayourfriend ! |
sarayourfriend
commented
Feb 22, 2021
|
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
|
The size of this snapshots file is alarming:
|
|
@gziolo Yes it still needs to be refactored. |
|
The snapshot tests are updated (and a new note added to the PR description). However, currently |
7 tasks
sarayourfriend
commented
Feb 24, 2021
packages/components/src/ui/button/test/__snapshots__/index.js.snap
Outdated
Show resolved
Hide resolved
32 tasks
sarayourfriend
force-pushed
the
add/hooks-button
branch
2 times, most recently
from
cb32e7e to
5866a5f
Compare
|
So the It almost certainly has something to do with TypeScript, right? But Adding a specific build script to be run by I was having the issue locally and Any help is appreciated! |
|
@sarayourfriend Maybe we can swap |
sarayourfriend
force-pushed
the
add/hooks-button
branch
from
49287a5 to
2ea9c03
Compare
7 tasks
sarayourfriend
force-pushed
the
add/hooks-button
branch
from
2ea9c03 to
1fe29b4
Compare
|
@ItsJonQ After our 🍐 yesterday I think our plan is the following for this PR:
Is that correct? |
ItsJonQ
approved these changes
Mar 3, 2021
There was a problem hiding this comment.
@sarayourfriend Yes! That plan sounds right to me :).
Approved 🚀 from me!
5 tasks
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.
Description
Adds new Button, ButtonGroup components. Unfortuantely they need to be added together because they depend on one another due to shared context.
Note: I had to change the
prefixprop to bepredue to an issue with types. I'm not sure how to resolve it but you can replicate it here in this TS Playground. The issue has something to do with the overlap in HTMLAttribute props and the prop names of the component itself. Sometimes these work (like href) and other times they don't. Not sure what the issue is but renaming the prop was the best workaround I could find for now.This PR also includes an update to
toMatchStyleDiffSnapshotthat @ItsJonQ and I came up with together. Instead of usinggetComputedStylewhich doesn't work with CSS variables in JSDom (jsdom/jsdom#1895) we use the stylesheets themselves and diff those. For now this works, we may run into some instances where this doesn't work and we'll have to do some refactoring, but it's already more robust than our previous approach.TODO:
Button.How has this been tested?
Storybook.
Screenshots
Types of changes
New feature
Checklist: