podman-desktop icon indicating copy to clipboard operation
podman-desktop copied to clipboard

chore: use checkbox component

Open deboer-tim opened this issue 1 year ago • 3 comments
trafficstars

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: Screenshot 2024-02-15 at 10 12 43 AM

After: Screenshot 2024-02-15 at 10 33 30 AM

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.

deboer-tim avatar Feb 15 '24 15:02 deboer-tim

@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 avatar Feb 21 '24 18:02 deboer-tim

@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 page.getByLabel('CheckBox name') and make the test pass again. Although, anyone writing the test trying to handle the checkbox will be facing the same issue over and over, so proper handling as rewrite the checkbox seems like viable solution.

odockal avatar Feb 26 '24 15:02 odockal

converting to draft

benoitf avatar Mar 12 '24 17:03 benoitf

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.

deboer-tim avatar May 09 '24 15:05 deboer-tim

@benoitf are you ok to merge?

deboer-tim avatar May 10 '24 13:05 deboer-tim

yes

benoitf avatar May 10 '24 15:05 benoitf