-
Notifications
You must be signed in to change notification settings - Fork 204
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
Update style of filter toggle buttons #6106
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6106 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 265 265
Lines 10207 10210 +3
Branches 2465 2467 +2
=======================================
+ Hits 10151 10154 +3
Misses 56 56 ☔ View full report in Codecov by Sentry. |
2cca4f2
to
2c26b4f
Compare
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.
Just added a small suggestion regarding aliasing props, but LGTM.
icon, | ||
description, | ||
active, | ||
setActive, | ||
testId, | ||
}: FilterToggleProps) { | ||
const IconComponent = icon; |
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.
Perhaps you can alias it inline when unpacking the props. If I remember correctly, we use that pattern frequently in frontend-shared.
icon, | |
description, | |
active, | |
setActive, | |
testId, | |
}: FilterToggleProps) { | |
const IconComponent = icon; | |
icon: IconComponent, | |
description, | |
active, | |
setActive, | |
testId, | |
}: FilterToggleProps) { |
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 point. Will do.
Update style of filter toggle buttons to align them with the current mocks. Also fix accessibility of the pressed state by using the `pressed` button prop to indicate whether the toggle is active. Part of #6006.
2c26b4f
to
9820efb
Compare
Update style of filter toggle buttons to align them with the current mocks.
Also fix accessibility of the pressed state by using the
pressed
button propto indicate whether the toggle is active.
Part of #6006.
Filter active:
This should use a minus rather than a plus icon, but we don't have one in the pattern library yet. I will add that and update this.
Filter inactive: