Allow category label definition at axis level #4506

andig · 2017-07-15T09:03:15Z

Currently category labels can only be fined globally. This implies that

the can only be one category axis
category labels are applied to all other axis types, e.g. time even if not used

This PR fixes category labels being passed through moment.js when time axis exists (part of #4498). Fiddle https://jsfiddle.net/andig2/b2a2ddsd/ shows js console warning that is removed by this PR.

Also allows defining multiple category axis which is currently not possible.

I have only limited experience with chartjs right now so I cannot be sure of cross-effects I'm not seeing- please review carefully.

etimberg

I like the idea of adding something to the documentation about this. One thing I will say is that having data.labels set does not always mean that the x axis will be a category axis. If the user, or chart defaults, set it to something else then setting data.labels won't change that.

I also think we should add unit tests to cover this as needed.

andig · 2017-07-15T12:14:48Z

I like the idea of adding something to the documentation about this.

My feeling is that the whole implicit/explicit logic should go into a more general section as it probably applies to other chart/aces types as well. I'd leave that to you as I don't understand the entire bigger picture (which settings are inherited, generate axes etc).

One thing I will say is that having data.labels set does not always mean that the x axis will be a category axis. If the user, or chart defaults, set it to something else then setting data.labels won't change that.

Will make this clear, please let me know about the first part though.

I'll look into tests, too.

etimberg · 2017-07-15T12:19:02Z

In terms of the explicit/implicit stuff, maybe it could live in http://www.chartjs.org/docs/latest/configuration/#global-configuration ? @simonbrunel any thoughts or preferences on this?

simonbrunel

Looks good to me, just a few minor changes. I think it's a dup of #3732, could be interesting to check why the other PR hasn't been merged (there is also one more code change).

I'm fine with labels under scale options, not sure I like the implicit/explicit terms though (maybe simply "data labels" vs "scale labels").

simonbrunel · 2017-07-16T10:18:46Z

test/specs/scale.category.tests.js

@@ -136,6 +136,37 @@ describe('Category scale tests', function() {
 		expect(scale.ticks).toEqual(mockData.yLabels);
 	});

+	it('Should generate ticks from the axis labels', function() {


Mock data and manual construction of the scale are not the most trustful way to setup unit tests. I think we should instantiate a real chart if that's possible (which should be almost always the case).

var chart = window.acquireChart({ type: 'line', data: { data: [10, 5, 0, 25, 78] }, options: { scales: { xAxes: [{ id: 'x', type: 'category', labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'] }] } } }); expect(chart.scales.x.ticks).toEqual(['tick1', 'tick2', 'tick3', 'tick4', 'tick5']);

It's even less code and more robust since you don't manually manipulate the scale to obtain the values to check.

simonbrunel · 2017-07-16T10:21:53Z

docs/axes/cartesian/category.md

-The category scale will be familiar to those who have used v1.0. Labels are drawn from one of the label arrays included in the chart data. If only `data.labels` is defined, this will be used. If `data.xLabels` is defined and the axis is horizontal, this will be used. Similarly, if `data.yLabels` is defined and the axis is vertical, this property will be used. Using both `xLabels` and `yLabels` together can create a chart that uses strings for both the X and Y axes.
+The category scale will be familiar to those who have used v1.0. If global configuration is used, labels are drawn from one of the label arrays included in the chart data. If only `data.labels` is defined, this will be used. If `data.xLabels` is defined and the axis is horizontal, this will be used. Similarly, if `data.yLabels` is defined and the axis is vertical, this property will be used. Using both `xLabels` and `yLabels` together can create a chart that uses strings for both the X and Y axes.
+
+Specifying any of the settings above implicitly defines the x axis as `type: category`if not defined otherwise. For more fine-grained control of category labels it is also possible to add `labels` as part of the category axis definition. Doing so does not apply the global defaults for bar charts.


Missing space between type: category and if not defined otherwise.

Why only "bar charts" in ... the global defaults for bar charts

simonbrunel · 2017-07-16T10:23:12Z

docs/axes/cartesian/category.md

@@ -1,6 +1,38 @@
 # Category Cartesian Axis

-The category scale will be familiar to those who have used v1.0. Labels are drawn from one of the label arrays included in the chart data. If only `data.labels` is defined, this will be used. If `data.xLabels` is defined and the axis is horizontal, this will be used. Similarly, if `data.yLabels` is defined and the axis is vertical, this property will be used. Using both `xLabels` and `yLabels` together can create a chart that uses strings for both the X and Y axes.
+The category scale will be familiar to those who have used v1.0. If global configuration is used, labels are drawn from one of the label arrays included in the chart data. If only `data.labels` is defined, this will be used. If `data.xLabels` is defined and the axis is horizontal, this will be used. Similarly, if `data.yLabels` is defined and the axis is vertical, this property will be used. Using both `xLabels` and `yLabels` together can create a chart that uses strings for both the X and Y axes.


I think I would remove the first sentence: The category scale will be familiar to those who have used v1.0., @etimberg

andig · 2017-07-16T13:03:17Z

@simonbrunel

not sure I like the implicit/explicit terms though

Change this to match global/local

Mock data and manual construction of the scale are not the most trustful way to setup unit tests

I've setup this test in line with all the other ones I've found. If to be changed then it should be throughout the board?

I think I would remove the first sentence

Removed

Why only "bar charts" in ... the global defaults for bar charts

Removed that. To be honest the global logic is cloudy to me. "labels" alone seems sufficient to generate a bar chart without even setting type?

Update I still need to check #3732 Whatever is wrong there would probably warrant another test, too.

andig · 2017-07-16T13:13:52Z

From #3732:

I don't think the condition needs to change. Changing it might cause problems for the horizontal bar chart where the y axis is the primary axis.

The solution to this might just mean we need to introduce the concept of a "primary" axis direction (horizontal vs vertical) for a chart. Most charts would have horizontal as a primary direction except for the horizontal bar chart. Making those changes is outside the scope of this change though.

It seems this notion is still open and outside the scope of this PR (or #3732 for that). I've left the original code untouched, #3732 has a change there.

Both PRs seem fair to merge imho.

simonbrunel · 2017-07-16T19:31:42Z

It's fine to write your new tests with acquireChart without changing others (can still clean them later). The other PR is quite old, may be better to gather all changes in this one and close #3732?

andig · 2017-07-16T19:53:36Z

@simonbrunel all changes have been gathered. If you can live with the test for now it's ready to merge and close the old one.

andig · 2017-07-16T19:55:04Z

I could also volunteer to look into converting some of the tests. However, quite a number of those are failing locally for me on OSX, especially the responsive ones?

simonbrunel · 2017-07-16T20:02:49Z

I would prefer to get your tests using the new way instead of the mock data before merging (mostly copy/paste code from my comment). Curious to know why the responsive tests are failing in your environment. We will be happy to review another PR with some tests upgraded :)

Are you sure we don't need this check?

etimberg · 2017-07-17T01:10:41Z

I have 4 tests that fail on Chrome on OSX only. I always just assumed they were because my machine is 5 years old and so it's just too slow to render properly because the error is Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL. Test failures outside those 4 would definitely be unexpected to me.

Regarding that extra check that Simon mentioned, the way to test if it is needed would be to create a chart with a vertical category axis that has labels specified in this new way and see if the tooltip shows the correct values. getLabelForIndex is only used inside of the tooltip code.

I will agree about the test using acquireChart since it's does make the test a little simpler and more robust.

andig · 2017-07-17T07:11:26Z

@etimberg moved the unit test failures to #4515

andig · 2017-07-17T09:29:16Z

Regarding that extra check that Simon mentioned, the way to test if it is needed would be to create a chart with a vertical category axis that has labels specified in this new way and see if the tooltip shows the correct values. getLabelForIndex is only used inside of the tooltip code.

I've had another look into this, see https://jsfiddle.net/andig2/3c7f3zxb/5/.

The current logic (before this PR) is already broken. If labels is specified, tooltip will show label of y axis tick. This is apparently (?) the expected behaviour. If yLabels is specified instead, tooltip will show value of y axis tick. This is imho already broken.

@simonbrunel @etimberg

My suggestion would be to merge this PR, close #3732 and open another issue for inconsistent behaviour of getLabelForIndex for category axis. I've also noticed that- in case of a bubble chart- getLabelForIndex for time axis also returns value instead of (formatted) label. Might be worth another issue?

etimberg · 2017-07-17T11:56:26Z

My suggestion would be to merge this PR, close #3732 and open another issue for inconsistent behaviour of getLabelForIndex for category axis.

I agree on that course of action since it keeps the PRs smaller and clearer. Thanks for updating the test. I've marked it as approved.

Allow category label definition at axis level

785c016

andig mentioned this pull request Jul 15, 2017

Howto Combined line/bar chart with x/y lines #4498

Closed

etimberg requested review from simonbrunel and etimberg July 15, 2017 11:50

etimberg requested changes Jul 15, 2017

View reviewed changes

simonbrunel requested changes Jul 16, 2017

View reviewed changes

Add test

fafda17

andig force-pushed the category-labels branch from 1a0e661 to fafda17 Compare July 16, 2017 12:56

Update docs

fbfbb03

Update test and docs

4488813

simonbrunel added the type: enhancement label Jul 16, 2017

simonbrunel added this to the Version 2.7 milestone Jul 16, 2017

Make test more robust

7e29719

etimberg approved these changes Jul 17, 2017

View reviewed changes

andig mentioned this pull request Jul 17, 2017

Category scale should show labels not values #4516

Closed

simonbrunel approved these changes Jul 19, 2017

View reviewed changes

simonbrunel merged commit 3bb31ca into chartjs:master Jul 19, 2017

andig deleted the category-labels branch July 21, 2017 08:24

simonbrunel mentioned this pull request Dec 2, 2017

[BUG] Chart.js grid lines and axes not working in v2.7.1 as they did in v2.4.0 #5017

Closed

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017

Allow category labels definition at scale level (chartjs#4506)

cfd0f41

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Allow category labels definition at scale level (chartjs#4506)

fd76231

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow category label definition at axis level #4506

Allow category label definition at axis level #4506

andig commented Jul 15, 2017 •

edited

Loading

etimberg left a comment

andig commented Jul 15, 2017 •

edited

Loading

etimberg commented Jul 15, 2017

simonbrunel left a comment

simonbrunel Jul 16, 2017 •

edited

Loading

andig Jul 17, 2017

simonbrunel Jul 16, 2017

simonbrunel Jul 16, 2017

simonbrunel Jul 16, 2017

andig commented Jul 16, 2017 •

edited

Loading

andig commented Jul 16, 2017

simonbrunel commented Jul 16, 2017

andig commented Jul 16, 2017

andig commented Jul 16, 2017

simonbrunel commented Jul 16, 2017

etimberg commented Jul 17, 2017

andig commented Jul 17, 2017

andig commented Jul 17, 2017

etimberg commented Jul 17, 2017

Allow category label definition at axis level #4506

Allow category label definition at axis level #4506

Conversation

andig commented Jul 15, 2017 • edited Loading

etimberg left a comment

Choose a reason for hiding this comment

andig commented Jul 15, 2017 • edited Loading

etimberg commented Jul 15, 2017

simonbrunel left a comment

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

andig Jul 17, 2017

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017

Choose a reason for hiding this comment

andig commented Jul 16, 2017 • edited Loading

andig commented Jul 16, 2017

simonbrunel commented Jul 16, 2017

andig commented Jul 16, 2017

andig commented Jul 16, 2017

simonbrunel commented Jul 16, 2017

etimberg commented Jul 17, 2017

andig commented Jul 17, 2017

andig commented Jul 17, 2017

etimberg commented Jul 17, 2017

andig commented Jul 15, 2017 •

edited

Loading

andig commented Jul 15, 2017 •

edited

Loading

simonbrunel Jul 16, 2017 •

edited

Loading

andig commented Jul 16, 2017 •

edited

Loading