-
Notifications
You must be signed in to change notification settings - Fork 3
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
[ENH] Added FormControlLabel
for clickable checkbox text
#63
Conversation
Signed-off-by: samadpls <[email protected]>
✅ Deploy Preview for neurobagel-query ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks @samadpls for the PR - but I think we need to talk a bit more about what the goal is here. We don't want to add a new label just to make the checkbox work. Maybe @rmanaem has some ideas here as well. But the main goal from a user perspective is "I can click outside the checkbox and the checkbox will still become checked". That can be achieved in a number of ways. Maybe you have some ideas?
<FormControlLabel | ||
control={ | ||
<Checkbox | ||
data-cy={`card-${datasetUUID}-checkbox`} | ||
checked={checked} | ||
onChange={() => onCheckboxChange(datasetUUID)} | ||
/> | ||
} | ||
label={datasetName} |
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.
This is a nice fix, but we don't want to add a label next to the checkbox, especially not with the datasetName
that you can already see on the dataset card. So I think we need to talk a bit more about what to do here. From a UX perspective, the nicest thing might be to make the entire card element clickable to set and unset the checkbox.
…madpls/react-query-tool into enh/clickable-checkbox-labels
@samadpls would you like to either discuss or propose a solution here without adding a new label to the checkbox? Essentially without this: |
I agree with your suggestion to avoid adding a new label to the checkbox. Perhaps we can add an |
Let's try the |
Apologies for any inconvenience caused. Due to time constraints, I won't be able to follow up on this specific PR.😢 |
No problem @samadpls, It's understandable. Thanks for contributing to the project! |
Changes proposed in this pull request:
FormControlLabel
for clickable checkbox textChecklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)skip-release
(to be applied by maintainers only)Closes #XXXX
For new features:
For bug fixes: