Add Checkbox question type#25
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
amy-corson-ibigroup
left a comment
amy-corson-ibigroup
left a comment
There was a problem hiding this comment.
Beautiful checkboxes! A few things but nothing blocking
miles-grant-ibigroup
force-pushed
the
checkboxes
branch
from
1f185b7 to
e530269
Compare
miles-grant-ibigroup
assigned
daniel-heppner-ibigroup and unassigned
miles-grant-ibigroup
daniel-heppner-ibigroup
left a comment
daniel-heppner-ibigroup
left a comment
There was a problem hiding this comment.
Let's avoid useEffect whenever possible!
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [selectedOptions] | ||
| ) |
There was a problem hiding this comment.
Don't use useEffect here. Just make a callback function that calls updateSelectedOptions and updateCallback and call that function from line 35 where you call updateSelectedOptions now
| [selectedOptions] | ||
| ) | ||
|
|
||
| const uuid = v4() |
There was a problem hiding this comment.
hmmm. Does this cause the id to change on every rerender? Like when a box is checked and unchecked?
| type="checkbox" | ||
| value={index} | ||
| /> | ||
| <span className={styles.label} id={`${option}-label`}> |
There was a problem hiding this comment.
If you are using option for the ID here then why not use it for the key on label too instead of index?
| className={styles.button} | ||
| defaultChecked={selectedOptions[index] === true} | ||
| disabled={disabled} | ||
| id={`${uuid}-${index}`} |
There was a problem hiding this comment.
why not just use option or option-index?
daniel-heppner-ibigroup
assigned
miles-grant-ibigroup and unassigned
daniel-heppner-ibigroup
|
Putting this on hold for now! |
3 participants
Curious for feedback on styling. Tried to optimize for longer answer lengths.