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

Add event handler to CheckBoxRow.js #50

Closed
pieroapretto opened this issue May 14, 2019 · 11 comments · Fixed by #111
Closed

Add event handler to CheckBoxRow.js #50

pieroapretto opened this issue May 14, 2019 · 11 comments · Fixed by #111
Assignees
Labels

Comments

@pieroapretto
Copy link
Collaborator

when clicking on checkbox label, checkbox 'checked' view should toggle.

@clementkng
Copy link
Collaborator

Hi, I'm interested in trying out this issue. How do issues get assigned?

@galbwe
Copy link
Collaborator

galbwe commented May 23, 2019

@clementkng thanks for expressing interest in the project! I'll invite you to collaborate and then assign you the issue.

@clementkng
Copy link
Collaborator

I'm not entirely sure what the expected behavior for this issue is. Is this saying that clicking the checkbox label i.e. label, vehicle transportation, etc. should toggle the checkbox the way clicking the form-check-input does? Or is this referring to something else?

@galbwe
Copy link
Collaborator

galbwe commented May 30, 2019

@clementkng Yes I believe you are correct. If someone clicks on the text "Building", "Vehicle Transportation", or "Infastructure Transportation", the checkbox to the left of that text should toggle.

While you are working on this component, it would also be nice to add a hover effect where the mouse cursor changes to a pointing hand when the user mouses over a checkbox or checkbox text.

@clementkng
Copy link
Collaborator

@galbwe Thanks, that sounds good. I've been looking at PRs in this repo, and was wondering if you would prefer that I fork off the repository or simply create branches off master when I make a pull request.

I've been able to get a solution working w/ pure Javascript, but since I'm pretty new to React, I'm not sure what's the best design decision for CheckBoxRow component ie add state? Is there a slack channel or some other channel to discuss this?

@galbwe
Copy link
Collaborator

galbwe commented Jun 1, 2019

@clementkng I don't have a preference for one way or the other, and we've been using both without any trouble so far. Use whichever method you are more comfortable with.

I'm new to React as well, so take this with a grain of salt - I would try to store the current combination of selections in the state of a parent component, maybe called CheckBox, and make CheckBoxRow a functional component that is passed event handlers and anything else it needs through props. @ASTPMeetup can you weigh in on this matter?

We have a slack channel in Code for KC's workspace. What is your email address? I'll reach out to one of the admins and get you added. If you are in the KC area, we also meet in person on Monday nights at the Code for KC meetup.

@clementkng
Copy link
Collaborator

@galbwe Thanks for letting me know, I'll try out the parent state approach. My email is clementdng@gmail.com. Unfortunately, I'm not in the KC area.

@clementkng
Copy link
Collaborator

@galbwe I've found an approach that allows you to create custom checkboxes that have the desired behavior here. I was thinking we could use this approach to customize CheckBoxRow and the new CheckBox component and tailor it back to the appearance of a typical React checkbox, but would this introduce any breaking changes? Overall, this seems cleaner than getting and setting a React checkbox via DOM manipulation, but there is increased overhead due to the new styled-components dependency.

@galbwe
Copy link
Collaborator

galbwe commented Jun 3, 2019

@clementkng I'm for it. It seems way cleaner than messing with the default stylings on a checkbox element. @ASTPMeetup will introducing styled-components as a dependency break anything with the scss stylings or the webpack build, to the best of your knowledge?

@clementkng
Copy link
Collaborator

@galbwe I made a PR for this issue, but I realize I forgot to add the hover effect for the event handler. Should I amend this PR or make a new one?

@galbwe
Copy link
Collaborator

galbwe commented Jun 5, 2019

@clementkng You can go ahead and modify this PR.

@krisstee krisstee linked a pull request Oct 25, 2020 that will close this issue
krisstee added a commit that referenced this issue Oct 25, 2020
…ndler

#50 - Add event handler for Checkboxrow.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants