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

Custom checkbox #292

Merged
merged 20 commits into from
Sep 13, 2024
Merged

Custom checkbox #292

merged 20 commits into from
Sep 13, 2024

Conversation

lkleuver
Copy link
Contributor

@lkleuver lkleuver commented Sep 3, 2024

Change checkbox so it has custom styling to allow for error state.

part of #263

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.

  • The border-color of the unchecked unfocused (not in error state) checkbox is too dark (Figma)
  • Please use the focus (border+glow) we also use for buttons for the focus state of checkboxes (See components in Figma)
  • When the warning is shown, the checkbox should have a error/600 red border. Currently it doesn't. (Figma)
  • The checkmark in design is a bit heavier then the current implementation (Figma)
  • The label element should have a top and border padding or margin of 1.5rem, so it is properly spaced to the alert and the submit button.
  • On small viewports, the checkbox's width is reduced, and the checkbox isn't square anymore.

Scherm­afbeelding 2024-09-03 om 13 04 22
Scherm­afbeelding 2024-09-03 om 13 13 20

frontend/lib/ui/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
@praseodym praseodym added the frontend Issues or pull requests that relate to the frontend label Sep 3, 2024
@Lionqueen94 Lionqueen94 mentioned this pull request Sep 3, 2024
9 tasks
Copy link
Contributor

@praseodym praseodym left a comment

Choose a reason for hiding this comment

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

Could you remove the commits to main that are duplicated in this branch?

@lkleuver
Copy link
Contributor Author

lkleuver commented Sep 5, 2024

  • The border-color of the unchecked unfocused (not in error state) checkbox is too dark (Figma)
  • Please use the focus (border+glow) we also use for buttons for the focus state of checkboxes (See components in Figma)
  • When the warning is shown, the checkbox should have a error/600 red border. Currently it doesn't. (Figma)
  • The checkmark in design is a bit heavier then the current implementation (Figma)
  • The label element should have a top and border padding or margin of 1.5rem, so it is properly spaced to the alert and the submit button.
  • On small viewports, the checkbox's width is reduced, and the checkbox isn't square anymore.

I've think I've addressed all issues, except the not showing the red border, I can't reproduce this.
If you can, could you check if the custom checkbox div has a class "has-error" ?

@lkleuver
Copy link
Contributor Author

lkleuver commented Sep 5, 2024

Could you remove the commits to main that are duplicated in this branch?

🤦

@jschuurk-kr
Copy link
Contributor

@lkleuver I don't get the red checkbox on the Voters and Votes page, only on the Differences page.

@lkleuver
Copy link
Contributor Author

  • it is present in the document
  • it does not have its css property display set to none
  • it does not have its css property visibility set to either hidden or collapse
  • it does not have its css property opacity set to 0
  • its parent element is also visible (and so on up to the top of the DOM tree)
  • it does not have the hidden attribute
  • if <details /> it has the open attribute

To summarize, styles aren't loaded by our testing library currently, we can either load them or focus RTL tests on checking for the correct classes/attributes.

Currently I've added the hidden attribute to the section that hides the checkbox. This works and circumvents loading styles. We could also check for the presence of "hidden" on the section, but this couples it a lot to the html structure.

@jorisleker
Copy link
Contributor

jorisleker commented Sep 11, 2024

There is still the 'checkbox remains checked' bug.

Reproduction steps:

  • On voters and votes enter some numbers that will result in a warning (ie A100, D100, E90, F10, H100)
  • press volgende
  • check the 'accept warnings' checkbox, don't press volgende
  • change your input to something that will trigger a warning (ie change E94, F6)
  • the checkbox disappears
  • press volgende.
  • you get a warning, the checkbox reappears and is checked (should be unchecked)

Styling + other behavior look good to me

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.

see last comment

@lkleuver
Copy link
Contributor Author

onChange didn't fire, changed it.
Might be a controlled input in the future.

jorisleker
jorisleker previously approved these changes Sep 12, 2024
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.

retested the bug I found earlier, has been resolved!

@lkleuver lkleuver dismissed jschuurk-kr’s stale review September 13, 2024 10:30

resolved by using click

@lkleuver lkleuver added this pull request to the merge queue Sep 13, 2024
Merged via the queue into main with commit 69ac604 Sep 13, 2024
7 checks passed
@lkleuver lkleuver deleted the custom-checkbox branch September 13, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues or pull requests that relate to the frontend
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants