Add checkbox and radio menu buttons#112821
Conversation
flutter-dashboard
bot
added
d: api docs
gspencergoog
force-pushed
the
menu_checkbox
branch
from
fcf455d to
b3b473f
Compare
gspencergoog
force-pushed
the
menu_checkbox
branch
3 times, most recently
from
1e8d601 to
b1c7a9a
Compare
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM!
Sorry it takes a little long for the code review, but I got a change to learn some new widgets and the whole structure of the Menu system which is very cool=)! Left some questions, just for my education and some minor comments. Thanks!
There was a problem hiding this comment.
Just curious is this IgnorePointer widget used to avoid the circular highlight for the checkbox? If so, do we still need to set the splashRadius to 0 below?
There was a problem hiding this comment.
Yes, it is there so that the checkbox doesn't get pointer events like hover, because I don't really want it being interacted with directly. You're right that the splash radius doesn't need to be zero, I'll remove that (although it doesn't hurt anything).
There was a problem hiding this comment.
nit: do we need this setting if the checkbox is constrained by the ConstrainedBox?
There was a problem hiding this comment.
This was my attempt to reduce its size without the ConstrainedBox, which didn't work. So yes, it can be removed.
There was a problem hiding this comment.
Yes, I'll remove the extra arguments here too.
There was a problem hiding this comment.
Just a question for my learning, is the manual_tests folder used to test the keyboard event, such as the shortcuts?
There was a problem hiding this comment.
I use the manual_tests folder to put any tests that I use when validating an implementation. Basically, like an example, but instead of something "reasonable", I try and make it use all of the features of a widget or system. Then I can try them out while I am developing, or use them to reproduce problems when they are reported. So, not just keyboard and shortcut events, but all kinds of things.
There was a problem hiding this comment.
Got it! Never used this folder before, thanks for the explanation:)
There was a problem hiding this comment.
nit: Not sure if MenuAnchor is precise enough, should we also mention CheckboxMenuButton? Same for the RadioMenuButton code sample below.
There was a problem hiding this comment.
You're so polite. :-). Yeah, that's just an error. It should be CheckboxMenuButton.
There was a problem hiding this comment.
I use the manual_tests folder to put any tests that I use when validating an implementation. Basically, like an example, but instead of something "reasonable", I try and make it use all of the features of a widget or system. Then I can try them out while I am developing, or use them to reproduce problems when they are reported. So, not just keyboard and shortcut events, but all kinds of things.
There was a problem hiding this comment.
You're so polite. :-). Yeah, that's just an error. It should be CheckboxMenuButton.
There was a problem hiding this comment.
I'll fix this one too.
There was a problem hiding this comment.
Yes, it is there so that the checkbox doesn't get pointer events like hover, because I don't really want it being interacted with directly. You're right that the splash radius doesn't need to be zero, I'll remove that (although it doesn't hurt anything).
There was a problem hiding this comment.
This was my attempt to reduce its size without the ConstrainedBox, which didn't work. So yes, it can be removed.
There was a problem hiding this comment.
Yes, I'll remove the extra arguments here too.
gspencergoog
force-pushed
the
menu_checkbox
branch
from
b1c7a9a to
c1ce4fb
Compare
gspencergoog
added
the
autosubmit
2 participants
Description
Adds
CheckboxMenuButtonandRadioMenuButtonto the set of buttons designed to work inside of a menu. Includes sample code and tests for both the buttons and the samples.Related Issues
Tests