-
Notifications
You must be signed in to change notification settings - Fork 99
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
Replace bg_color with bgcolor #742
Conversation
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 with a couple of comments/recommendations
@@ -79,7 +79,7 @@ class Demo(HasTraits): | |||
Group( | |||
Item( | |||
"plot", | |||
editor=ComponentEditor(size=size, bgcolor=bg_color), | |||
editor=ComponentEditor(size=size, bgcolor=bgcolor), |
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.
FTR : ComponentEditor
is the only place where I see someone set bgcolor
on an enable AbstractWindow
- and it sets bgcolor
, not the deprecated bg_color
.
@@ -96,7 +96,6 @@ def _create_plot_component(): | |||
# Attributes to use for the plot view. | |||
size = (650, 650) | |||
title = "Scatter plot with selection" | |||
bg_color = "lightgray" |
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.
why not pass this to the ComponentEditor
- like what we do in the other examples
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.
I can add that here in this PR. I deleted it just because previously this bg_color
variable was just unused entirely.
It makes sense to actually use it though
@@ -87,7 +87,6 @@ def _create_plot_component(): | |||
# Attributes to use for the plot view. | |||
size = (650, 650) | |||
title = "Scatter plot with selection" | |||
bg_color = "lightgray" |
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.
same comment as above - why not pass this to the ComponentEditor
, like we do with the other examples
Here's the original commit where |
ref: enthought/enable#816 (review)
This PR removes uses of the soon to be removed, deprecated
bg_color
trait from enable. For consistency it renames all uses ofbg_color
in the codebase withbgcolor
(even just temp variables), in order to keep consistent. Note, all changes occur in example code