codeforkansascity / meep

Mapping Energy Efficiency & Public Health
MIT License
10 stars 20 forks source link

Add event handler to CheckBoxRow.js #50

Closed pieroapretto closed 3 years ago

pieroapretto commented 5 years ago

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

clementkng commented 5 years ago

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

galbwe commented 5 years ago

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

clementkng commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@clementkng You can go ahead and modify this PR.