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.
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
Sync Label component with PVC's Label component #1797
Sync Label component with PVC's Label component #1797
Changes from 9 commits
5c9018a
6a66da2
fae2072
73e2911
7c5bcc0
96aa767
e94b1ec
2e9587b
4754dd1
0863146
7bfc14b
8c903c5
cbf7dc6
cb8469e
1974be8
9ad5680
ebb8756
106a854
9ff1905
590c7a2
f494349
0e6490b
f015e1f
a9af0ba
83ddc6d
72084d4
7b69c5f
8fe7f81
6286cf0
9510286
659e6c8
4f612a5
08b3e11
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We can remove these once #1785 is merged
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 like this prop is called
scheme
in the code.Also, I'm curious if we should call this prop
variant
to be consistent with the Button API: https://primer.style/react/drafts/Button2#buttonThere 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.
Yup, this is just a mistake. Good catch!
Agreed on changing to
variant
too. I was just copying the PVC API.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 wonder if we can use the
variants
utility from styled-system to clean up all theget()
calls here.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.
Maybe something like 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.
Ah - yea, this feels cleaner. I'll give it a shot 👍
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.
Should we use relative units like PVC too?
2em
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.
The border-radius isn't relative to anything - it's just a really high number to make it perfectly round.
border-width is also not relative to anything, it's just 1px.
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.
It seems like this is equivalent to:
Is there a reason we need to have this proxy
Label
component? What if we exported the styled component directly?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.
This is leftover from a more complex implementation where we handled a
leadingVisual
. We can just use the styled component directly.Good catch.