-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add support for widgets in toolbars #359
Conversation
Codecov Report
@@ Coverage Diff @@
## master #359 +/- ##
==========================================
+ Coverage 35.16% 35.19% +0.02%
==========================================
Files 463 463
Lines 25772 25799 +27
Branches 3830 3834 +4
==========================================
+ Hits 9064 9079 +15
- Misses 16326 16336 +10
- Partials 382 384 +2
Continue to review full report at Codecov.
|
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.
Overall looks good. Just some questions and a small nit
@@ -245,6 +263,9 @@ class _Tool(HasTraits): | |||
# The toolkit control. | |||
control = Any() | |||
|
|||
# The toolkit control id. | |||
control_id = None |
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.
Is this not purely a Wx-ism as stated in the previous 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.
Yep - I'd like to get rid of it, but it was already implicitly there. This is drive-by clean-up to make it explicit (then we can work out how to get rid of it in a separate PR at a later time).
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.
Okie dokie.
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.
LGTM
Proper Pyface-level support for
Action
s which contain widgets. Support is very basic, but works.