Skip to content

fix: fixed checkbox check on label click #2633

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

waaseyzulfiqar
Copy link

No description provided.

@tordans
Copy link
Contributor

tordans commented May 14, 2025

Can you share the code from the browser inspector that this creates?

I am concerned that this will create duplicate label tags because OriginalCheckboxWidget (which is https://github.com/rjsf-team/react-jsonschema-form/blob/573980dbf91f9891905c7d3612359c2c90c53ee3/packages/chakra-ui/src/CheckboxWidget/CheckboxWidget.tsx#L64C8-L75 already has label property which should only be hidden when null according to https://github.com/rjsf-team/react-jsonschema-form/blob/573980dbf91f9891905c7d3612359c2c90c53ee3/packages/chakra-ui/src/components/ui/checkbox.tsx#L25

@waaseyzulfiqar
Copy link
Author

I've made the change and updated the code to pass label={null} to OriginalCheckboxWidget instead of an empty string. This should prevent the widget from rendering its own label and avoid any duplicate elements in the DOM.

I’m currently unable to run the app locally to test it fully, but the change is pushed — could you please have a look and let me know if it works as expected?

@jlewin
Copy link
Contributor

jlewin commented May 21, 2025

I'm not sure if it's helpful to chime in here, but I've been looking forward all week to taking my new dev env for a spin on this PR. On my machine and with this PR I'm seeing an error on the consent form:

image

seemingly from line 180:

image

This approach in this PR is very similar to how I would have tried solving this ticket. With more review, however, I see a few issues that I'll share:

  1. The RJSF checkbox component expects to render the label, provides a functional mechanism to support linking the label and checkbox, and feels like a more natural pattern and ownership.
  2. Using this RJSF functionality eliminates the need for the more classic label/htmlFor id route and the extra overhead.
  3. The RJSF checkbox already creates and wraps itself in a label that seemingly can't be bypassed. Thus, we'll end up creating two labels for this custom checkbox implementation, with one wired up via id/htmlFor, and the other inactive one wrapping the checkbox.

In investigating the errors, and trying to understand the rendering mechanics at play, I worked up a solution that tackles these problems and succeeds without the id/htmlFor. It feels weird to toss another option into the mix, but figured I'd share that the approach works and might be an alternative option.

@jlewin
Copy link
Contributor

jlewin commented May 21, 2025

For reference, here's the alternative approach

https://github.com/maproulette/maproulette3/compare/main...jlewin:maproulette3:jl/issue-2633?expand=1

checkbox

@CollinBeczak
Copy link
Collaborator

For reference, here's the alternative approach

https://github.com/maproulette/maproulette3/compare/main...jlewin:maproulette3:jl/issue-2633?expand=1

I'm a fan of this approach. If you agree @waaseyzulfiqar, could you implement this change or let @jlewin know to make a new pr linked to this one?

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.

4 participants