Skip to content

Conversation

@michalfita
Copy link
Contributor

@michalfita michalfita commented Oct 25, 2023

Purpose

To address #763 and improve overall UX of using Checkbox in the application.

Scope

  • The implementation
  • Examples

To do

  • Add label for showing selection over whole Checkbox
  • Create grouping for MultiChoice to make application logic easier
  • Example combining Checkbox and RadioButton

Notes

Commits bring changes incrementally, so I suggest reviewing them one by one and consider rejections based on changes in them as maybe not all PR would need to be rejected at once.

For now I do draft for first impression where I'm heading. I'm trying to not break original API, however, some alignment with RadioButton in future would do good.

@michalfita michalfita force-pushed the issue/763/improve-checkbox-ux branch from 7fbb78b to cfdc5dd Compare October 25, 2023 15:06
@gyscos
Copy link
Owner

gyscos commented May 31, 2024

Thanks for the work!

I merged main into this branch (solving minor conflicts that arose when views became Send), and it should now be closer to ready.

One remark: the example now shows 2 ways to make groups of checkboxes: the "manual way" (for toppings) and the "managed way" (for extras). To keep the example simple, I think we can get rid of the manual way and only show how to use the multichoice group.

Instead, we could include a couple of "non-grouped" checkboxes, to show that it's still an option for single options. Or not, since that's already covered in the "list_view" example. In that case, I would rename the checkbox example to checkbox_group or multichoice_group.

@michalfita
Copy link
Contributor Author

@gyscos Oh man, it's been a while since I've been on this... Finding time to go back to this may be close to impossible in a next few weeks.

@michalfita
Copy link
Contributor Author

@gyscos I forgot about this PR until I went back to Cursive for something I'd like to implement. I've aligned the code with main again and simplified the example a little.

@michalfita michalfita marked this pull request as ready for review October 10, 2025 17:37
Copy link
Owner

@gyscos gyscos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR!

Just a few minor details.

Also, are the two examples different?

@michalfita
Copy link
Contributor Author

Also, are the two examples different?

Now, examples are even more different. I think they'd work well for someone trying to consider various options how to use checkboxes.

Comment on lines +56 to +57
.cloned()
.map(|item| item.value)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to switch the two around, so it only clones the Arc<T>, which means you don't need to implement Clone for Item<T> anymore.

@gyscos gyscos merged commit 344c6c1 into gyscos:main Oct 17, 2025
1 check passed
@gyscos
Copy link
Owner

gyscos commented Oct 17, 2025

Thanks again for the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants