-
Notifications
You must be signed in to change notification settings - Fork 0
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 Checkbox question type #25
base: dev
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful checkboxes! A few things but nothing blocking
1f185b7
to
e530269
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid useEffect whenever possible!
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[selectedOptions] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use option
or option-index
?
Putting this on hold for now! |
Curious for feedback on styling. Tried to optimize for longer answer lengths.