elastic / elastic-charts

https://elastic.github.io/elastic-charts/storybook
Other
369 stars 118 forks source link

Improved pixel padding logic #1179

Open nickofthyme opened 3 years ago

nickofthyme commented 3 years ago

Improve domain pixel padding logic implemented in #1145 to account for edge cases and situations that would apply excessive padding.

@markov00 good call here— with Nick, we discussed the way to invoke the screenspaceMarkerScaleCompressor such that it returns the needed value, whether padding is needed on the top, bottom, or both (eg. bar chart with both + and - values). So far so good.

@nickofthyme what we discussed yesterday is still good, as in, you're guaranteed to get sufficient padding, but the way I wrote those 4 lines, it's possible to get too much padding:

  • nicing the raw domain may already ensure sufficient padding, while, if we add the padding and then nice, then it may lead to a much taller pad than would be needed otherwise, so one would think, it's worth attempting to nice and check the padding first (screenspace distance between raw domain max and last niced tick domain value)
  • if the nicing of the raw domain doesn't lead to sufficient padding, even then is still possible that adding the padding first (with the little function drawn up yesterday) then nicing leads to a too tall padding—maybe adding a smaller bit to the domain, then do the nicing may yield good results

There is another problem too: sometimes it's not sensible (legal) to extend the domain. Eg. if we show some ratio, and the user knows it can't ever be more than 100%, and our value is 95% and padding would make it, say, 120% with nicing, then it'll be a nonsensical axis.

So, this is what could be done:

  • compute the niced domain min/max from the raw domain min/max and the desired tick count
  • observe the difference between the raw domain max and niced domain max, I'll call it domainGrowth (min is the same, so I won't be repetitive with that)
  • this domainGrowth will have a screenspace height in pixels, and that should be subtracted from the required domainPaddingPx
  • this requires a small linear solver (basically a couple of lines with the four basic operators) which I'm happy to write, we should just first clarify the requirements

So I guess, what I'm proposing is that the aesthetically pleasing top pad has two parts:

  • the effect of nicing, as it already often provides some clearing; this is also the Y axis domain
  • the remaining, visually necessary padding, so the nicing plus this together gives us the required pixel padding

A key point is that the axis is extended only due to nicing, but not for the remainder of the padding. Why?

  • it avoids most of the illegal axes (eg. nicing up 95% to 100% is OK)
  • it avoids the issue of double nicing (if we were going to extend the axis past the niced domain max, then it means that another nicing may be needed)

=============

tl;dr if I clearly expressed the issue here, and everyone is OK with this approach (we include the nicing domainGrowth as partial fulfilment toward the full domainPaddingPx; and we only extend the actual axis to the nicing of the original domain, but not into the rest of the padding) then pls. give the go ahead and I can add couple-liner calculation that

  • takes as input the raw domain max, and the niced domain max(*)
  • full height in pixels
  • required domainPaddingPx And returns the scale, and the actual (residual) domain padding in pixels, ie. the part beyond the nicing.

=============

(*) [finer point here] this assumes that the addition of pixel padding doesn't materially change the height of the Y axis. Reason: as you vary the nicing, it varies the leftover pixel padding. Which in turn varies the actual cartesian projection area height. Which in turn influences how many Y ticks fit. Which in turn modifies the desired tick counts. Which in turn impacts the nicing, since nicing is a function of the increment, therefore the tick count. If we want to solve this, it's a lovely problem and it needs that we make screenspaceMarkerScaleCompressor nice-aware. Ie. it would compute not just the pairwise scales but the pairwise niced scales. I'd love to work on that, and generally, nicing needs to be integral to the solving process, not a post-processing step

_Originally posted by @monfera in https://github.com/elastic/elastic-charts/pull/1145#discussion_r639122742_

monfera commented 3 years ago

Thanks Nick for extracting this comment 🙌