-
Notifications
You must be signed in to change notification settings - Fork 259
[Fixes #763] Introduced labelled checkboxes #765
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
Conversation
7fbb78b to
cfdc5dd
Compare
|
Thanks for the work! I merged main into this branch (solving minor conflicts that arose when views became 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. |
|
@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. |
|
@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 |
gyscos
left a comment
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 again for the PR!
Just a few minor details.
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. |
| .cloned() | ||
| .map(|item| item.value) |
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.
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.
|
Thanks again for the work! |
Purpose
To address #763 and improve overall UX of using
Checkboxin the application.Scope
To do
CheckboxMultiChoiceto make application logic easierCheckboxandRadioButtonNotes
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
RadioButtonin future would do good.