chore: use checkbox component #6023
Conversation
deboer-tim
requested review from
axel7083 and
lstocchi
and removed request for
a team
deboer-tim
force-pushed
the
checkboxes
branch
from
16b67e9
to
c752b72
Compare
|
@odockal helped investigate. Although for humans and vitest it's fine, the E2E tests are unable to click a Checkbox because it's a native sr-only checkbox underneath a FA SVG - to playwright, there is always something hiding the real checkbox. We will likely have to rewrite the Checkbox to avoid this. |
|
@deboer-tim A workaround could be in this case, to use parent element (div) that has a label for specific checkbox, so we can get the element with |
Pull request was converted to draft
|
converting to draft |
deboer-tim
force-pushed
the
checkboxes
branch
2 times, most recently
from
010a703
to
50a2358
Compare
I noticed several places where we're using a standard blue <input type="checkbox" /> instead of our styled Checkbox. Run Image is the biggest user. This change originally caused failures in the E2E tests because our Checkbox was incompatible with Playwright, since the input in this checkbox component was neither visible nor Aria compliant. I wasted lots of time earlier this year trying to build a custom checkbox that was aria-compliant, but the fix was much simpler after carefully reading the Playwright requirements here: https://playwright.dev/docs/actionability#visible The input checkbox cannot be hidden or 0 size, and cannot be 'behind' the div icon. However, it can be opacity-0 and on top using absolute positioning and changing the z-order by swapping the order of elements. Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
Rebased with changes to checkbox, moving back to ready for review. @benoitf, @lstocchi (or others) please take another look or re-confirm your approval by merging. Thanks to @odockal for his help. This change originally caused failures in the E2E tests because our Checkbox was incompatible with Playwright, since the input in this checkbox component was neither visible nor Aria compliant. I wasted lots of time trying to build a custom checkbox that was aria-compliant, but the fix was much simpler after carefully reading the Playwright requirements here: https://playwright.dev/docs/actionability#visible The input checkbox cannot be hidden or 0 size, and cannot be 'behind' the div icon. However, it can be opacity-0 and on top using absolute positioning and changing the z-order by swapping the order of elements. |
|
@benoitf are you ok to merge? |
|
yes |
What does this PR do?
I noticed several places where we're using a standard blue instead of our styled Checkbox.
Run Image is the biggest one, just requires the ability to set a class on Checkbox for spacing.
Screenshot / video of UI
Before:
After:
What issues does this PR fix or reference?
N/A
How to test this PR?
Unit tests still work; page through Run Image form to confirm styling.