Category axis doesn't consider offscreen labels when requesting space #3223
Conversation
|
screencaps / gifs? is this smooth now? |
| @@ -134,10 +134,10 @@ export class Category extends Axis<string> { | |||
| * @param {Scales.Category} scale - The scale being downsampled. Defaults to this Axis' scale. | |||
| * @return {DownsampleInfo} an object holding the resultant domain and new stepWidth. | |||
| */ | |||
| public getDownsampleInfo(scale: Scales.Category = <Scales.Category> this._scale): DownsampleInfo { | |||
| public getDownsampleInfo(scale: Scales.Category = <Scales.Category> this._scale, domain = scale.invertRange()): DownsampleInfo { | |||
There was a problem hiding this comment.
I didn't know you could reference previous args in later default args. Neat
| @@ -370,7 +367,6 @@ export class Category extends Axis<string> { | |||
| // hide ticks and labels that overflow the axis | |||
| this._showAllTickMarks(); | |||
| this._showAllTickLabels(); | |||
| this._hideOverflowingTickLabels(); | |||
There was a problem hiding this comment.
Why don't we need this anymore?
src/scales/categoryScale.ts
Outdated
| @@ -50,6 +50,19 @@ export class Category extends Scale<string, number> implements TransformableScal | |||
| this._outerPadding = Category._convertToPlottableOuterPadding(0.5, d3InnerPadding); | |||
| } | |||
|
|
|||
| /** | |||
| * Return a clone of this category scale without any included values providers. | |||
There was a problem hiding this comment.
I'm glad you're encapsulating this into a method. Can you include in this comment some info about what IS included in the partial "clone" of this scale.
|
There's some jank for how this behaves when pan/zoom and downsampling are enabled: Zoom suddenly stops because the axis grows: Layout jankily jumps around because the specific labels being drawn (and therefor the height) change while panning: Thoughts? |
|
@hellochar I would assume once we do vertical labels and truncate them at fixed height, the height of individual labels won't change when zooming and panning? |
@helenkg this still won't fix the problem because some labels might be at the full height & truncated while others are not close to the truncation limit @hellochar in general I'm not a fan of the behavior shown in the above gifs; what if you waited until pan/zoom completed before redrawing the layout? |
|
@adidahiya not a fan of tying re-compute to panzoom interactions since that adds a lot of complexity. Let me try debouncing requestedSpace() and see how that feels |
|
@hellochar can you elaborate on the code complexity just a bit? I think it could work out really well actually from a user's perspective. Also curious if @pkwi has any thoughts |
|
throttling doesn't work. Also, with maxWidth and 90 degree angles, it still has some jank:
@pkwi thoughts here? |
improve commentDemo: quicktests | fiddle |
|
@adidahiya here's "wait until pan is finished to recalculate":
Definitely feels better. The zoom issue is still there but I think I'm okay with leaving that in until people complain. |
…-labels Conflicts: package.json plottable.js
Merge branch 'develop' into xzhang/axis-should-not-consider-offscreen-labelsDemo: quicktests | fiddle |
|
Okay, i think i finally get how this works. I was just missing the fact that your pass in the value of |
|
I'm gonna merge this because I want to pull these changes in to test sheared ticks. If there are still more changes to address the "jank", that can be a followup PR |
themadcreator
deleted the
xzhang/axis-should-not-consider-offscreen-labels
branch
Category Axis will now only render/consider the ticks that will actually be drawn. This fixes a bug where pan/zoomed category axes might have a lot of unnecessary whitespace.