Story 2191: Button Badge Component#2276
Conversation
| @@ -0,0 +1,32 @@ | |||
| .badge-button__radio { | |||
There was a problem hiding this comment.
should the cursor be a pointer?
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
looking pretty good! just a couple of questions there, but I'm pre-approving for your convenience
There was a problem hiding this comment.
@jlchilders11 This is great work!
Just one additional thing I want to flag for your consideration:
From my understanding, these badge buttons are meant to be used as a group rather than as standalone instances. Can you consider setting up a parent radiogroup component (or adapt the current _badge_button to become a group), similar to _post_filter.html? This would let us specify an aria-label and other accessibility attributes at the group level, making the overall accessibility configuration more meaningful and complete.
julioest
left a comment
julioest
left a comment
There was a problem hiding this comment.
Other folks pointed out things I had in mind - Approved!
jlchilders11
force-pushed
the
jc/badge-button
branch
from
28f7878 to
524f29d
Compare
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
I just left a small request to provide color for the focus outline ring – otherwise LGTM!
|
|
||
|
|
||
| .badge-button:has(.badge-button__radio:focus-visible) { | ||
| outline: auto; |
There was a problem hiding this comment.
I think this is producing a black outline, let's provide it with a color:
outline: auto -webkit-focus-ring-color;
jlchilders11
force-pushed
the
jc/badge-button
branch
from
cd826a9 to
a3550f0
Compare
5 participants
Issue: #2191
Summary & Context
Changes
_badge_button.htmlandbadge-button.cssPlease list any potential risks or areas that need extra attention during review/testing
None
Screenshots
Self-review Checklist
Frontend