Skip to content

include margins in the label translate #402

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Fil
Copy link
Contributor

@Fil Fil commented May 14, 2021

fixes #375

@Fil Fil requested a review from mbostock May 14, 2021 17:21
@Fil Fil force-pushed the fil/translate-label-375 branch from 71ed111 to 2bf5776 Compare August 18, 2021 15:40
@Fil Fil force-pushed the fil/translate-label-375 branch from 2bf5776 to bcecaef Compare August 19, 2021 22:16
@Fil
Copy link
Contributor Author

Fil commented Aug 19, 2021

rebased

@mbostock
Copy link
Member

mbostock commented Sep 7, 2021

A little explainer would be nice here… I’m finding it hard to follow what the logic was vs. what it should be. I feel like the current behavior was intentional (put the x-axis label as far right as possible to give it space); now I’m trying to decide what the new behavior should be, if the current one is not desirable.

@Fil
Copy link
Contributor Author

Fil commented Sep 7, 2021

The idea is to position the axis label relative to the "tip of the axis", instead of having it float to the far end. In the case of the OP, that gives:

before

Capture d’écran 2021-09-07 à 07 47 22

after

Capture d’écran 2021-09-07 à 07 47 57

@mbostock
Copy link
Member

mbostock commented Sep 7, 2021

But what is the default label margin of 20?

@Fil
Copy link
Contributor Author

Fil commented Sep 7, 2021 via email

@mbostock
Copy link
Member

mbostock commented Sep 7, 2021

Because normally there isn’t a second axis? It’s matching the 20 from here, I presume.

plot/src/plot.js

Lines 118 to 121 in 07cf90b

marginTop = Math.max((xAxis === "top" ? 30 : 0) + facetMarginTop, yAxis || fyAxis ? 20 : 0.5 - offset),
marginRight = Math.max((yAxis === "right" ? 40 : 0) + facetMarginRight, xAxis || fxAxis ? 20 : 0.5 + offset),
marginBottom = Math.max((xAxis === "bottom" ? 30 : 0) + facetMarginBottom, yAxis || fyAxis ? 20 : 0.5 + offset),
marginLeft = Math.max((yAxis === "left" ? 40 : 0) + facetMarginLeft, xAxis || fxAxis ? 20 : 0.5 - offset)

@Fil Fil marked this pull request as draft September 7, 2021 15:24
@Fil
Copy link
Contributor Author

Fil commented Sep 7, 2021

I see now that this branch works only in a few cases, and is far from working in general.

I've made an interactive here https://observablehq.com/d/79d3d39812dd3441 ; it seems to me that the behavior of the two first charts is desirable, i.e. that the label should be tied to the tip of the axis. But it would need to work in the four faceting possibilities (no faceting, x, y, x+y), for labelAnchor=left, and also for the vertical axis (which I haven't touched here either).

Maybe an alternative would be to have a configurable label.shift? (label.offset is for the orthogonal direction). I'm closing this for the time being.

@Fil Fil closed this Sep 7, 2021
@mbostock
Copy link
Member

mbostock commented Sep 7, 2021

Nice notebook. That helps! 👍 It’s definitely tricky and I expect we do want some tweaks to the default behavior as well as likely new options.

@Fil Fil deleted the fil/translate-label-375 branch June 7, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

marginRight in y facet does not translate label of x
2 participants