Add support for hiding axis when a dataset visibility is toggled

[davesalomon]

I'd like to have an option to hide an axis on a line graph when a dataset visibility is toggled off/on (by clicking the legend).

I've attempted to add support for this for my exact use case; I'm happy to build it out further if needed.

JS Fiddle example: https://jsfiddle.net/y0whvnvk/99/
Note that the 'Foo' dataset is hidden by default. Toggling it on will display the axis. Toggling it back off will re-hide the axis.

[kurkle]

To me, it would appear logical for an axis with no referencing visible datasets to be hidden (by default in next major?) . Maybe display could be set to 'auto'? I dislike another configuration option for this.
Just my 5 cents.

[davesalomon]

I agree entirely. Showing an axis for hidden data is confusing, and just adds clutter.

I just didn't want to change any default behaviour as part of this PR 😄

[simonbrunel]

I also prefer display: 'auto' instead of introducing a new option which is redundant with display:

• display: true (default): always display the axis
• display: false: never display the axis
• display: 'auto': display the axis only if there is at least one visible associated dataset

The scale should also not participate to the layout when hidden so everywhere we check scale.options.display, we also need to apply this new logic (thus the _isVisible() refactor).

We also need to consider situations where all datasets are hidden: how the chart behaves in that case?

[simonbrunel]

@etimberg @nagix what do you think about that feature and the suggested API?

[davesalomon]

Thanks for the review; much tidier. Recommended changes made, new Fiddle at https://jsfiddle.net/jyontsh6/. If this seems like a good thing to do, I'll start working on some tests 👍

[simonbrunel]

display is also used in fit(), which I guess is why the hidden scale still consumes layout space:

[davesalomon]

Reverted, and change made to handle .fit() 👍

Conflicts resolved

[simonbrunel]

Thanks @davesalomon, it looks good to me! I would go ahead and write unit tests for this new feature.

[etimberg]

I like it. Just needs tests

[nagix]

This looks good unless it changes the current default behavior.

[davesalomon]

Tests added; checking that scale.ctx._calls is empty seemed one of the easiest ways of validating that the axes hadn't been drawn (though I'm not sure this is right...). I was originally checking that the _isVisible() call returned the expected value, but that wasn't really then doing the check whether the axes has been drawn or not.

Review comments worked in as well.

To confirm, this PR doesn't change any default behaviour; axes.display default is still true, this adds a new option of auto.

[simonbrunel]

... checking that scale.ctx._calls is empty seemed one of the easiest ...

I agree, later we should find a way to prevent the call of scale.draw() if the scale is not visible, but for now I think that's enough and better than just checking _isVisible(). I also agree with @nagix suggestions to also test the scale geometry.

[davesalomon]

Tests checking the geometry added, and ....length > 0).toBe(true) tweaked also. Many thanks for the feedback 👍

[simonbrunel]

Thanks @davesalomon

[davesalomon]

👍, my pleasure, and thank you again for all your work going in to this excellent library!