Skip to content
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.

Sign up for GitHub

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

unwanted change in spacing for marginal x and y plots #1862

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

unwanted change in spacing for marginal x and y plots #1862

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad87b3d
Even out spacing between marginal x and y plots
joelostblom Nov 3, 2019
2e3ed30
Take figure size into account when setting marginal spacing
joelostblom Nov 4, 2019
18ef78f
Blacken single quotes to double
joelostblom Nov 6, 2019
3af5b47
Do not adjust spacing width on figures that autoscale
joelostblom Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/python/plotly/plotly/express/_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,7 +1330,7 @@ def init_figure(args, subplot_type, frame_list, nrows, ncols, col_labels, row_la
main_size = 0.84

row_heights = [main_size] * (nrows - 1) + [1 - main_size]
vertical_spacing = 0.01
vertical_spacing = 0.012 * 600 / args["height"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're checking for width being None below, shouldn't we check for height being None here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, instead of multiplying two constants, could you just use 7.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments @marthacryan . I'm a bit removed from this PR since it was a while since I created it, but your suggestions here and in the other comment sounds good so feel to make those edits (any others you see fit) directly on this branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/me waves at @joelostblom

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, hello Greg! Nice to see you here =)

elif args.get("facet_col_wrap", 0):
vertical_spacing = 0.07
else:
Expand All @@ -1343,7 +1343,8 @@ def init_figure(args, subplot_type, frame_list, nrows, ncols, col_labels, row_la
main_size = 0.84

column_widths = [main_size] * (ncols - 1) + [1 - main_size]
horizontal_spacing = 0.005
width_adjustment = 1 if args["width"] is None else 600 / args["width"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability / ease of code comprehension, I'd love to have:

  • A comment for both of these explaining why these constants are being used here and why we're adjusting for width/height
  • More consistency between the width and the height adjustments. It wasn't immediately clear to me that they were doing the same thing

horizontal_spacing = 0.012 * width_adjustment
else:
horizontal_spacing = 0.02
else:
Expand Down