Skip to content
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

Implement custom radiobuttons + choicelist component #435

Merged

Conversation

Lionqueen94
Copy link
Contributor

@Lionqueen94 Lionqueen94 commented Oct 8, 2024

Description

  • Refactored current styling of custom checkbox to make it easier to use (no manual div toggling needed) and to make it suitable to use with radio buttons as well. Thanks @oliver3 for the help!
  • Added radio button and intermediate checkbox styling.
  • Added ChoiceList component (fieldset) to group multiple inputs.

What to test

  • Is the behaviour of the checkbox and radio inputs as desired
  • Is the styling of the radio inputs and checkbox (still) as designed
  • Is it ok that you sometimes have to click a radio button twice to check it (is this normal?) @jorisleker

@Lionqueen94 Lionqueen94 marked this pull request as ready for review October 8, 2024 22:13
@Lionqueen94 Lionqueen94 requested review from jorisleker and a team as code owners October 8, 2024 22:13
@Lionqueen94 Lionqueen94 requested review from praseodym, lkleuver, cikzh, oliver3 and jschuurk-kr and removed request for a team October 8, 2024 22:13
Copy link
Contributor

@jorisleker jorisleker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on MacOS (Chrome, Safari, Firefox):

  • ✅ styling of radio's looks okay (all states)
  • ✅ styling and behavior of checkbox is still ok. (can't retest a group of checkboxes though, because we don't have that yet)
  • ❌ when hovering, the label has a hand/pointer cursor, the radio has a default cursor. Could we use pointer for both?
  • ❌ padding/margin left of radio's should be removed; radio's should left align with elements above and below it
  • ❌ when using tab to navigate through the page, it is now impossible to continu to the submit button from the choice group. Please compare to vanilla html form, behavior should be same. (might be fixed in Navigating form sections and focus state #305?)
  • ❌ having to click a radio twice is not expected behavior. Don't know exactly when/how this happens, but it should not. Clicking a radiobutton or it's label should immediately change it's state to checked.

@Lionqueen94
Copy link
Contributor Author

Lionqueen94 commented Oct 9, 2024

Tested on MacOS (Chrome, Safari, Firefox):

* ✅ styling of radio's looks okay (all states)

* ✅ styling and behavior of checkbox is still ok. (can't retest a group of checkboxes though, because we don't have that yet)

* ❌ when hovering, the label has a hand/pointer cursor, the radio has a default cursor. Could we use pointer for both?

* ❌ padding/margin left of radio's should be removed; radio's should left align with elements above and below it

* ❌ when using tab to navigate through the page, it is now impossible to continu to the submit button from the choice group. Please compare to vanilla html form, behavior should be same. (might be fixed in [Navigating form sections and focus state #305](https://github.com/kiesraad/abacus/issues/305)?)

* ❌ having to click a radio twice is not expected behavior. Don't know exactly when/how this happens, but it should not. Clicking a radiobutton or it's label should immediately change it's state to checked.

Fixed the cursor, padding and margin in ee2a9bb
The navigate issue should indeed be fixed by merging my other pr
Edit: Fixed double click issue in 9149666
Please retest 👼

…trolled input

Cleaned up code some more and added comments
@jorisleker
Copy link
Contributor

LGTM now @Lionqueen94! Let's make sure to check the tab/navigation issue after merging this to main

@jorisleker jorisleker added this pull request to the merge queue Oct 10, 2024
Merged via the queue into main with commit 36ff8ee Oct 10, 2024
7 checks passed
@jorisleker jorisleker deleted the 371-implement-custom-radiobuttons-+-choicelist-component branch October 10, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement custom radiobuttons + choicelist component
3 participants