-
Notifications
You must be signed in to change notification settings - Fork 98
Change default padding based on axis titles #735
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
Changes from all commits
8bef6cf
74fda24
0f48723
934d804
e45fd13
656f2d5
4021982
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| """ | ||
| from numpy import array, transpose | ||
|
|
||
| from traits.api import Bool, Enum, Instance, Property | ||
| from traits.api import Bool, Enum, Instance, Int, observe, Property, Union | ||
| from enable.api import color_table | ||
|
|
||
| from .abstract_overlay import AbstractOverlay | ||
|
|
@@ -207,11 +207,71 @@ class DataView(OverlayPlotContainer): | |
| #: Background color (overrides Enable Component) | ||
| bgcolor = "white" | ||
|
|
||
| #: Padding defaults. | ||
| padding_top = 50 | ||
| padding_bottom = 50 | ||
| padding_left = 50 | ||
| padding_right = 50 | ||
| # Padding defaults. These were converted to properties as a hacky fix for | ||
| # enthought/chaco#564 | ||
| #: Top Padding default | ||
| padding_top = Property( | ||
| observe='y_axis.[title,orientation], x_axis.[title,orientation]' | ||
| ) | ||
| #: Bottom Padding default | ||
| padding_bottom = Property( | ||
| observe='y_axis.[title,orientation], x_axis.[title,orientation]' | ||
| ) | ||
| #: Left Padding default | ||
| padding_left = Property( | ||
| observe='y_axis.[title,orientation], x_axis.[title,orientation]' | ||
| ) | ||
| #: Right Padding default | ||
| padding_right = Property( | ||
| observe='y_axis.[title,orientation], x_axis.[title,orientation]' | ||
| ) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I handle all 4 cases because it is possible to set axis orientation to "top" or "bottom" as well
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as an example, this code:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extra padding on the bottom on this branch (80 rather than 50) I'm not a huge fan of. This is due to the simplistic nature of just adding space if we have an axis title. As you can see, at the top we need extra space so axis title doesn't overlap with plot title. Similarly left right we need it (hence the original issue), because axis ticks / values take up some of the spacing and sort of push the axis title further away from the plot. The x axis doesn't usually have this problem because of the orientation of the numbers on the ticks. In other words the text for In reality an ideal solution would calculate explicitly the extent of the ticks, and the size of the axis title and allocate spacing to perfectly fit what we need. The awkward thing about this code feels like it would live in a draw method inside the axis itself, but the DataView is where the padding is held. Should the Axis take control over the DataViews padding? That feels wrong. Perhaps we just make the assumption that padding_bottom can be 50 regardless of the presence or absence of an axis title? We were just assuming 50 always before anyway. |
||
|
|
||
| _padding_top = Union(None, Int()) | ||
| _padding_bottom= Union(None, Int()) | ||
| _padding_left = Union(None, Int()) | ||
| _padding_right = Union(None, Int()) | ||
|
Comment on lines
+229
to
+232
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rahulporuri
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this deserves an issue in traits - traits should explicitly complain during class definition instead of at runtime.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, for my own understanding we want to catch a user setting |
||
|
|
||
| def _find_padding(self, side): | ||
| SIDE_TO_TRAIT_MAP = { | ||
| "top": "_padding_top", | ||
| "bottom": "_padding_bottom", | ||
| "left": "_padding_left", | ||
| "right": "_padding_right", | ||
| } | ||
| if getattr(self, SIDE_TO_TRAIT_MAP[side]) is not None: | ||
| return getattr(self, SIDE_TO_TRAIT_MAP[side]) | ||
| else: | ||
| if self.y_axis: | ||
| if (self.y_axis.title and self.y_axis.orientation == side): | ||
| return 80 | ||
| if self.x_axis: | ||
| if (self.x_axis.title and self.x_axis.orientation == side): | ||
| return 80 | ||
| return 50 | ||
|
|
||
| def _get_padding_top(self): | ||
| return self._find_padding("top") | ||
|
|
||
| def _get_padding_bottom(self): | ||
| return self._find_padding("bottom") | ||
|
|
||
| def _get_padding_left(self): | ||
| return self._find_padding("left") | ||
|
|
||
| def _get_padding_right(self): | ||
| return self._find_padding("right") | ||
|
|
||
| def _set_padding_top(self, value): | ||
| self._padding_top = value | ||
|
|
||
| def _set_padding_bottom(self, value): | ||
| self._padding_bottom = value | ||
|
|
||
| def _set_padding_left(self, value): | ||
| self._padding_left = value | ||
|
|
||
| def _set_padding_right(self, value): | ||
| self._padding_right = value | ||
|
|
||
| border_visible = True | ||
|
|
||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.
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.
if explicitly set (or if
paddingtrait is sett, which then explicitly sets these), that value will always be used. Setting sets the shadow traits (with leading underscore), and in the getters we explicitly check if the shadow trait is set and return it. Otherwise we then check for axis titles and adjust as needed either returning 80 or 50 as a default.Note I am just now considering if a user explicitly sets one off these traits to 0 I dont think it will work as then we will not enter theif getattr(self, MAP[side])block. This is a bug that needs fixing.EDIT: fixed