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

[ENH] Added FormControlLabel for clickable checkbox text #63

Closed
wants to merge 12 commits into from
Closed

[ENH] Added FormControlLabel for clickable checkbox text #63

wants to merge 12 commits into from

Conversation

samadpls
Copy link
Contributor

@samadpls samadpls commented Mar 8, 2024

Changes proposed in this pull request:

  • Added FormControlLabel for clickable checkbox text

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for neurobagel-query ready!

Name Link
🔨 Latest commit 99db6a4
🔍 Latest deploy log https://app.netlify.com/sites/neurobagel-query/deploys/66312531ecf5190008c44508
😎 Deploy Preview https://deploy-preview-63--neurobagel-query.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@surchs surchs left a 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?

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +35 to +43
<FormControlLabel
control={
<Checkbox
data-cy={`card-${datasetUUID}-checkbox`}
checked={checked}
onChange={() => onCheckboxChange(datasetUUID)}
/>
}
label={datasetName}
Copy link
Contributor

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.

@surchs
Copy link
Contributor

surchs commented Mar 19, 2024

@samadpls would you like to either discuss or propose a solution here without adding a new label to the checkbox? Essentially without this:
image

@surchs surchs added the _community [BOT ONLY] PR label for community contributions. Used for tracking label Mar 19, 2024
@samadpls
Copy link
Contributor Author

@samadpls would you like to either discuss or propose a solution here without adding a new label to the checkbox? Essentially without this:
image

I agree with your suggestion to avoid adding a new label to the checkbox. Perhaps we can add an onClick event to the Card component or explore other options you recommend.

@samadpls samadpls marked this pull request as draft April 4, 2024 22:48
@rmanaem rmanaem self-requested a review April 5, 2024 16:10
@rmanaem
Copy link
Contributor

rmanaem commented Apr 9, 2024

I agree with your suggestion to avoid adding a new label to the checkbox. Perhaps we can add an onClick event to the Card component or explore other options you recommend.

Let's try the onClick event and explore other solutions if that doesn't work out as expected.

@rmanaem rmanaem added the pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1) label Apr 9, 2024
@samadpls
Copy link
Contributor Author

samadpls commented May 9, 2024

Apologies for any inconvenience caused. Due to time constraints, I won't be able to follow up on this specific PR.😢
However, It was a great experience working with all of you during the GSoC selection process!😊

@samadpls samadpls closed this May 9, 2024
@rmanaem
Copy link
Contributor

rmanaem commented May 9, 2024

No problem @samadpls, It's understandable. Thanks for contributing to the project!
Please stay in touch and feel welcome to contribute again to our repos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
_community [BOT ONLY] PR label for community contributions. Used for tracking pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[ENH] Make text associated with checkbox clickable
3 participants