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.

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

Adding automargin support to plot titles #6428

Merged
merged 63 commits into from
Mar 15, 2023
Merged

Adding automargin support to plot titles #6428

merged 63 commits into from
Mar 15, 2023

Conversation

hannahker
Copy link
Contributor

@hannahker hannahker commented Jan 6, 2023

This pull request adds 'automargin' support for layout.title components. If layout.title.automargin=true, then either the top or the bottom margin space will expand such that the plot title is visible in the plotting area.

To-dos:

  • Add hard-coded support for title.automargin
  • Fix failing tests for pie charts
  • Determine whether margin should be added to top or bottom based on layout.title.y
  • Push margin depending on title font size
  • Set title.y=1 and title.yref='paper' defaults when title.automargin=true (in supplyDefaults stage)
  • Allow padding to be added to both top and bottom, irrespective of yanchor
  • Have title avoid overlap with container when yref='paper'
  • Write Jasmine tests for yref='paper'
  • Have title avoid overlap with plot area when yref='container'
  • Write Jasmine tests for yref='container'
  • BUG: Fix interactions with automargined x-axes when title is on the bottom of the plot area (and test interactions with other elements adding margin to the top)
  • NA: Set defaults in defaults setting phase
  • OUT OF SCOPE: Account for multi-line titles (not just estimation based on font size) (maybe relevant multi-line axis titles unable to push margins #4299) -- also see Multi-line titles don't respond to title.yanchor #6472
  • BUG: Fix errors in unnecessary redrawing when yref='paper'

API Questions:

  • Should title.automargin support multi-line titles? Looking at this codepen, there seem to be some issues with multi-line positioning, see Multi-line titles don't respond to title.yanchor #6472
    A: Currently out of scope given existing bug.

  • Should title.automargin allow the title to overlap with the plot itself? Or does title.automargin only ensure that the title is visible? Ex. what if title.y=0.6? Should the margin then expand to take up the top 40% of the plot area?
    A: This depends on title.yref. When the title is 'paper' referenced, then title.automargin can't do anything to impact potential overlap with the plot area, as the plot area is also 'paper' referenced. When the title is 'container' referenced, then title.automargin will increase the top or bottom margin to avoid overlap with the plot area.

  • If a developer manually specifies title.yref='container' and title.automargin, then which one 'wins'?
    A: N/A. In this case title.automargin will have an impact.

  • layout.title.y=1 + layout.title.yanchor='bottom' / layout.title.y=0 + layout.title.yanchor='top --> should automargin handle these cases so that the title is visible? Logically, I don't think it should be.
    A: Assuming that title.yref='container', the title should not be visible in these cases as it is outside of the plot area. If title.yref='paper', then this would be a common way to pin the title to either the top or bottom of the plot area.

  • What if layout.title.y='auto' and there isn't enough manually-defined top margin to fit the title? (ref: ""auto" places the baseline of the title onto the vertical center of the top margin"). Should we add in logic here so that the top margin will expand to 2x the size of the title? It doesn't seem intuitive to the developer in this case that the top margin will have this artificial padding.
    A: When title.automargin=true, we can set the default title.y=1, which is a more logical default and would avoid this behaviour.

  • How will title.automargin behaviour be different when yref='container' vs yref='paper'?
    A: If yref='container', then title.automargin will push margins to ensure that the title doesn't overlap with the plot area. If yref='paper', then title.automargin will only push margins to ensure that the title doesn't overlap with the edges of the container.

  • Should title.automargin potentially push out the left or right margins?
    A: No, this implementation will only support spacing added to top or bottom margins.

Technical Questions:

  • How should the x and y input parameters to the Plots.autoMargin function be specified? I would expect that these would be just title.x and title.y. In testing, seems only to work when y is 1 or 0. (ref: "x, y: plot fraction of the anchor point.")
  • How do different automargined features interact with each other when they're pushing out the same margin? Ex. when title.automargin applies bottom margin as well as the x-axis?

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

@hannahker very nice improvement.
Please find my comments below.
Thank you!

src/components/titles/index.js Outdated Show resolved Hide resolved
test/image/mocks/zzz-automargin-title.json Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
src/plot_api/subroutines.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Excellent, I think we finally got there! 💃
I know there's still an issue with multiline titles - as shown in the zzz-automargin-title-paper-multiline mock - but since this was already buggy before this PR we can address that in a patch.

@hannahker hannahker merged commit 7dd2a88 into master Mar 15, 2023
@hannahker hannahker deleted the automargin-title branch March 15, 2023 17:31
@Alexboiboi
Copy link

This is so good for multiline titles, thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants