plot icon indicating copy to clipboard operation
plot copied to clipboard

auto margins

Open Fil opened this issue 2 years ago • 12 comments
trafficstars

Alternative to #1714. The axis mark sets the value of marginLeft (etc.) not to a number, but to a special value ("auto", not a symbol if we want to keep this usable from outside); the actual numeric value of that special value is computed in createDimensions—where the scales’ types and domains are already materialized

TODO:

  • [x] facet axes?
  • [x] find a “good” heuristic (consider the scales’ domain, ticks, tickFormat…)
  • [x] consider labelAnchor ("center" needs more space)
  • [x] use approx. text metrics instead of string.length
  • [x] account for the possibility of a centered axis label
  • [x] fit & test monospace (see #1877 for easier tests?)

closes #1451 closes #383 closes #1859

Fil avatar Jun 25 '23 16:06 Fil

This feels almost ready; needs a bit of manual testing (and more snapshot tests to capture more ranges of ticks?). In particular with the auto mark.

cc @tophtucker

Fil avatar Sep 14 '23 15:09 Fil

Can you regenerate the snapshots so we can more easily review the impact of this change? Thank you. 🙏

mbostock avatar Sep 14 '23 15:09 mbostock

Here's a playground to develop tests: https://observablehq.com/@observablehq/auto-margins-1722

Fil avatar Sep 14 '23 15:09 Fil

Computing all the ticks is probably more adequate, but it is also quite a bit more complicated, since the ticks are computed from the instantiated scales. I've tried to approximate it by doing half-way measures (like, apply tickFormat if given… but on what ticks?) and it seemed worse. My conclusion was that either we have a light system (this one, to which we could add a few common cases such as type: "log"), or we go full-in and measure the actual text that will be rendered, by running a (fake) axisY mark with all the options?

Fil avatar Sep 14 '23 16:09 Fil

Yes, it’s complicated. 🤔

mbostock avatar Sep 14 '23 16:09 mbostock

As discussed, we want to initialize (but not fully render) the axis text mark for y scales; this will give us the text channel which we can use to infer appropriate left & right margins.

mbostock avatar Sep 20 '23 17:09 mbostock

I've implemented the new strategy. Still needs a bit of work:

  • ~~clean up the code~~
  • ~~fix the two test cases mentioned above~~
  • ~~keyword check on "auto"~~ (no more keywords!)
  • ~~consider auto for top and bottom margins~~

Fil avatar Sep 22 '23 17:09 Fil

I think this is good to go! Does it need documentation?

Fil avatar Sep 27 '23 15:09 Fil

Yes I feel good about the result: the tests are all looking great and the only changes are improvements. I think the algorithm is correct. The implementation can certainly be better.

Fil avatar Sep 27 '23 21:09 Fil

One thing that bugged me is that monospaceWidth and defaultWidth are not aligned; maybe there's still time to change this by making monospace chars 60 instead of 100 — it would be a slightly breaking change, but it would make the two lengths comparable, which is something that was needed here.

Fil avatar Sep 27 '23 21:09 Fil

There was still a bug with the aspectRatio option.

Here's a "normal" scenario under which the ticks depend on the margins, when the chart has a set aspectRatio. Suppose that with margins of 40, the y axis has ticks such as "1,000", "1,000.5", "1,001". Adding margins on left and right makes the x range smaller, thus giving us a smaller range for y too (because aspectRatio), thus removing the 1000.5 tick. Now, these eventual ticks ("1,000", "1,001") would have fitted with the 40px marginLeft. But we're keeping the 60px.

This is captured with the new marginAspectRatio test.

Fil avatar Sep 28 '23 11:09 Fil

Probably worth resurrecting this one! 😅

mbostock avatar Jul 28 '24 23:07 mbostock

bump

AidMen avatar May 19 '25 06:05 AidMen