removing material from checkbox

[Jialecl]

No description provided.

[Jialecl]

The visual changes are caused by a bug present in the previous version of the checkbox.
To colour the check a coloured span appears behind the check which is cut from an SVG.
When the checkbox is disabled and checked the white span can be seen through and affect the colour of the checkbox.

[GomezIvann]

Summary of things I saw along the way

1. Disabled components should not gain the focus:

2. The space between label and checkbox should mark as selected the checkbox when clickable. The cursor changes correctly to pointer, but when clicked, doesn't change checkbox's state.

3. I think we can take advantage of this refactor and add some more jests tests. For example:
   • "Checkbox renders with correct text" is no longer necessary since we have visual tests.
   • Check keyboard accessibility.
   • Check disabled state (and avoid bugs such as point 1).
   • Check correct values for aria attributes and role.
   • Check that the value sent in onSubmit event is correct.
   • (Any more tests you can think of)

[GomezIvann]

More comments regarding the component:

1. The gap between the label and the checkbox doesn't change the state (hover and active) of the checkbox input.
2. Doble click on the label selects the text.

[GomezIvann]

We missed some functionality:

• With the current implementation, our label checks the checkbox input but doesn't give it the focus. We must implement that, which is how it would work if we had to use the native checkbox input with a label and htmlFor property.
  • Add onClick event to the label and give the focus to the checkbox (ref.current.focus();)
• Then, we must guarantee that the container, which has the "change value" functionality, doesn't steal the focus from the checkbox. This can be achieved by adding an onMouseDown to the main container with event.preventDefault().
• And finally, we need to give the focus to the checkbox when he clicks in the gap. This should be as simple as adding ref.current.focus(); to the change value handler.
• With the previous three points, the handleLabelDblClick handler should not be necessary.
• Optionally, if you want to avoid unnecessary calls to the focus() method, you can check if the checkbox already has the focus, like this: document.activeElement !== ref?.current. This should be a problem but may be more precise.

[Jialecl]

• Then, we must guarantee that the container, which has the "change value" functionality, doesn't steal the focus from the checkbox. This can be achieved by adding an onMouseDown to the main container with event.preventDefault().

In regards to this I find it unnecessary, a div without tabIndex=0 is never focusable.
Also removes the functionality to select the label.

[GomezIvann]

• Then, we must guarantee that the container, which has the "change value" functionality, doesn't steal the focus from the checkbox. This can be achieved by adding an onMouseDown to the main container with event.preventDefault().

In regards to this I find it unnecessary, a div without tabIndex=0 is never focusable. Also removes the functionality to select the label.

Agree!

[GomezIvann]

One final appointment:

• We need to also test aria-labelledby and aria-required properties.