feat(theme/Button): implement new DS theming #10315
Conversation
✅ ethereum-org-website-dev deploy preview ready
|
TylerAPfledderer
force-pushed
the
feat/button-new-ds-variants
branch
from
c47be22 to
7669ef0
Compare
TylerAPfledderer
requested review from
samajammin,
wackerow,
corwintines,
pettinarip,
minimalsm,
jmcook1186 and
nhsz
as code owners
TylerAPfledderer
force-pushed
the
feat/button-new-ds-variants
branch
from
7669ef0 to
efbdf50
Compare
| * Prevent React warning that does not recognize `isSecondary` on DOM | ||
| * while still sending prop to the theme config | ||
| */ | ||
| const styles = useStyleConfig("Button", { ...props, isSecondary }) |
There was a problem hiding this comment.
This approach allows us to retain the Chakra Button component with all its defaults for ARIA and baseline styles that are not in the theme config.
There was a problem hiding this comment.
Awesome! loved the multiline buttons, great job!
As a general feedback:
- we merged the nested semantic tokens PR so we will need to update those values in this PR
- I see a difference in height between the outline and the other variants. We are not counting the borders. To fix this problem, I'd set a transparent 1px border on the other variants to get the same height.
- [minor] the button's height in the DS is
42px. Would be nice if we could get that exact size. I'm not sure where is the problem because I see that we are using the correct font size and line height (1.6) cc @nloureiro
| _focusVisible: { | ||
| outline: "4px solid", | ||
| outlineColor: "primaryHover", | ||
| outlineOffset: -1, | ||
| }, |
There was a problem hiding this comment.
Out of the scope of this PR, but wondering if we could put this somewhere to reuse it in the rest of the components.
There was a problem hiding this comment.
Same with the transition rules. We want the same animation timings for everything.
Only if it make sense, if not, ignore it.
There was a problem hiding this comment.
@pettinarip I think adding objects in components.utils file will be the simplest here. 🤔
There was a problem hiding this comment.
Just want to callout that the sold and outline variants have different height. There are 2pxs difference because the solid one doesn't count the border that the outline has.
Maybe we need to define that border in the solid variant as transparent to keep both heights equal.
The rest is looking good for me.
@pettinarip This is reflected in the DS, as a border is indeed declared in all the styles that I could see. I made the update. I also added a tweak in line-height that was not the same as in the DS to ensure further consistency. |
Description
Part of the Implementation of the DS v1
Adds updated theming for the
Buttoncomponent per the new DSRelated Issue
Closes #10186
Closes #10020
Closes #8048