-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Shortlist button focusable #5228
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5228 +/- ##
=======================================
Coverage 82.40% 82.41%
=======================================
Files 409 409
Lines 32041 32042 +1
Branches 5109 5109
=======================================
+ Hits 26403 26406 +3
+ Misses 4125 4123 -2
Partials 1513 1513 ☔ View full report in Codecov by Sentry. |
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 works, but I wonder if there's an easier solution - if we switch this label to be an actual button with form="planned_form"
(and adjustments accordingly) then I don't think the submit button in the planned_form is even needed - perhaps whoever did this originally didn't know buttons in a form can actually be linked to a different form?
e5c3b4c
to
f279d21
Compare
Thanks for the feedback, I thought there would be a more powerful reason to use input and label. So I swapped it for a button and it seems to be working =) Screen.Recording.2024-10-23.at.15.11.49.mov |
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 good apart from one removal
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.
Great :)
This fix allows users using keyboards to be able to focus the label.btn--shortlist element to shortlist a report.
95a5b2e
to
a6ebf9a
Compare
Bug found while working on this PR: #5208
This PR changes the markup of
label.btn--shortlist
and swaps it forbutton.btn--shortlist
, that way the element is focusable and can also be interacted via keyboard.[Skip changelog]