Allow spanning data gaps for radar charts #5075

flaurida · 2017-12-22T20:07:28Z

Allow spanning gaps with missing data in radar chart
Support multiple font colors for point labels

Edit(SB): smart links to associated tickets

etimberg

I think the code is fine. I'm not sure if it would be helpful to add a test or not. @simonbrunel thoughts?

The docs in https://github.com/chartjs/Chart.js/blob/master/docs/charts/radar.md should be updated for the new property. I think that's as simple as copying the info from https://github.com/chartjs/Chart.js/blob/master/docs/charts/line.md

etimberg · 2017-12-22T20:49:45Z

docs/axes/radial/linear.md

@@ -104,7 +104,7 @@ The following options are used to configure the point labels that are shown on t
 | Name | Type | Default | Description
 | -----| ---- | --------| -----------
 | `callback` | `Function` | | Callback function to transform data labels to point labels. The default implementation simply returns the current string.
-| `fontColor` | `Color` | `'#666'` | Font color for point labels.
+| `fontColor` | ``Color/Color[]`` | `'#666'` | Font color for point labels.


[nit] looks like the backticks got duplicated on each side

Sorry about that, just pushed a fix up

flaurida · 2017-12-22T20:57:23Z

Sorry for the typos - moving too quickly. Will try to slow down, just wanted to see if we could get this fixed

etimberg · 2017-12-22T21:03:36Z

@flaurida no worries!

flaurida · 2017-12-22T21:07:23Z

Looks like my test is failing - I was having issues running tests locally, let me see what is up.

flaurida · 2017-12-22T21:49:47Z

Having a rough time getting this test to pass - tried logging out to the console and didn't see anything, so hard to know exactly which property to examine that has spanGaps. Any words of wisdom?

etimberg · 2017-12-22T22:05:35Z

@flaurida I think the correct check is expect(meta._model.spanGaps).toBe(true);

flaurida · 2017-12-22T22:06:32Z

I tried that and sadly not the case

flaurida · 2017-12-22T22:07:26Z

Pushed it up, maybe something is different for me locally?

etimberg · 2017-12-22T22:21:07Z

That was my bad, it should be meta.dataset._model.spanGaps. Sorry about that

simonbrunel

Looks great, just a minor change! To make simpler the review process for future PRs, I would avoid mixing multiple features and/or fixes.

simonbrunel · 2017-12-23T10:38:14Z

src/scales/scale.radialLinear.js

@@ -268,8 +268,11 @@ module.exports = function(Chart) {

 				// Keep this in loop since we may support array properties here
 				var pointLabelFontColor = valueOrDefault(pointLabelOpts.fontColor, globalDefaults.defaultFontColor);
+				var pointLabelFontColorFormatted = helpers.isArray(pointLabelFontColor) ?


Could be simply:

var pointLabelFontColor = helpers.valueAtIndexOrDefault(pointLabelOpts.fontColor, i, globalDefaults.defaultFontColor);

ok, thanks!

simonbrunel · 2017-12-23T10:46:38Z

Ideally, we would need a unit test that checks if the line is correctly drawn when spanGaps is true because the only test we have doesn't ensure that this feature works as expected.

flaurida · 2017-12-23T17:07:47Z

Thanks for the review! Happy to write that test. Is there an example somewhere I could look to? This is my first time contributing so still not that used to the codebase. Sorry :/

etimberg · 2017-12-23T17:55:06Z

@flaurida I would recommend a test spec like the ones in https://github.com/chartjs/Chart.js/tree/master/test/fixtures/plugin.filler with a JSON file that creates a radar chart and then the accompanying PNG image. You can generate the PNG image using http://www.chartjs.org/docs/latest/developers/api.html#tobase64image

@simonbrunel would have more tips since he's the one who generated these tests originally.

flaurida · 2017-12-23T20:39:54Z

I see, so do I just need to create the JSON file and the image? What else needs to be done? Thanks for being so prompt 😊

simonbrunel · 2017-12-24T01:35:48Z

@flaurida everything explained in #3988. You should disable (hide) all features that are not related to your PR (e.g. legend, scales, grid, etc...) and thus your PNG should display only what you want to test (the line continuity). I think this fixture is a good base for your case: you would need 2 JSON for the spanGaps feature (when true and false)

Though, I don't think you will be able to use that method for the font color because image based tests when displaying texts don't work well between different browsers.

flaurida · 2017-12-26T16:49:50Z

Hi @simonbrunel I keep trying with these picture generation tests, but for some reason the images look the same. What am I missing? Locally it does matter whether I set spanGaps to true, but for some reason that is not carrying over to the PNG files

flaurida · 2017-12-26T17:14:38Z

In addition, I am writing the test for the font colors of the point labels, and am having a hard time figuring out where the correct property lives. Whenever I try to console log the test output, there are so many nested key value pairs that I don't know where to begin. Thank you for your patience - any advice how to find the correct item?

flaurida · 2017-12-26T17:22:08Z

Here is what I have so far, but I don't think that I'm keying into the correct property:

	it('should pass an array of point label font colors', function() {
		var fontColors = ['black', 'black', 'pink', 'black', 'pink'];

		var chart = window.acquireChart({
			type: 'radar',
			data: {
				datasets: [{
					data: [10, 5, 0, 25, 78]
				}],
				labels: ['label1', 'label2', 'label3', 'label4', 'label5']
			},
			options: {
				scale: {
					pointLabels: {
						fontColor: fontColors
					}
				}
			}
		});

		var meta = chart.getDatasetMeta(0);
		expect(meta.dataset._scale.pointLabels.fontColor).toEqual(fontColors);
	});

simonbrunel · 2017-12-26T20:50:18Z

src/controllers/controller.radar.js

@@ -50,6 +51,8 @@ module.exports = function(Chart) {
 				// Model
 				_model: {
 					// Appearance
+					// This option gives lines the ability to span gaps
+					spanGaps: dataset.spanGaps ? dataset.spanGaps : options.spanGaps,


This doesn't work because if both options are defined, we want priority for the dataset one, but if dataset.spanGaps is false, then we use options.spanGaps. It should be spanGaps: (dataset.spanGaps !== undefined ? dataset.spanGaps : options.spanGaps)

flaurida · 2017-12-28T23:31:08Z

Hi @simonbrunel thanks for the review - I fixed the issue there as well as in the line controller. However, I am still seeing the issue where the generated png files are the same. In addition, can you help me figure out which key to test for the array of colors? What I have so far is shown above. Thanks :)

simonbrunel · 2017-12-29T09:08:49Z

I don't think the issue is with the tests but rather with how null values are interpreted in a radar chart. Maybe null is handled differently than NaN and point._model.skip, which is used to create gaps, is always false? (traveling so can't debug your code unfortunately)

@etimberg any idea/reference on how to test the font color as array since we can't use image for that?

etimberg · 2017-12-29T14:10:45Z

Regarding null, it gets interpreted as 0 here https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.radialLinear.js#L446-L449

which causes https://github.com/chartjs/Chart.js/blob/master/src/controllers/controller.radar.js#L115 to set skip to false in the model.

For the font colour as an array, you could use one of the tests that use the mock context and track the calls for the point labels but that's fairly hard to maintain.

flaurida · 2017-12-29T22:26:54Z

Awesome thanks for the responses! So if it is difficult to test the array of font colors should we skip that test then? And I tried passing NaN instead of null into the test for the photo and it did not work. I can try again though.

etimberg · 2017-12-29T23:05:12Z

NaN can’t load from JSON so that might be part of the issue

simonbrunel · 2017-12-29T23:18:26Z

I think null as 0 is a bug and should be fixed, what do you think @etimberg?

etimberg · 2017-12-29T23:26:39Z

Yup, we should fix that

flaurida · 2017-12-30T04:11:36Z

Alright thanks so much you two - I would like to get this PR in as soon as we can, so what do I need to do to make that happen?? Let me know :) Happy almost New Year 🎉 !

flaurida · 2018-01-08T16:51:53Z

@etimberg @simonbrunel Just following up on this - ideally would like to have this functionality as it is important in the app we are building. Happy to help get it done, just let me know the scope?

flaurida · 2018-01-16T15:22:43Z

@etimberg @simonbrunel any words of wisdom? Please let me know what I can do to help get this done!

simonbrunel · 2018-01-16T15:30:47Z

Sorry for the no reply - happy '18 to you as well :) We first need to fix how radar origin coordinates are computed in order to support null/NaN data values. I started to look at it but it's not as easy as I thought so I need more time (which I don't have a lot right now) to investigate how to do that without breaking existing projects.

I don't think this PR will land in 2.7.2.

flaurida · 2018-01-16T16:49:38Z

Thanks so much for the response! I understand that there is an issue that needs to be fixed. Is there any sense of how long that might take? Ideally I would like to resolver this by in about a month, is that possible? Please let me know if I can help!

benmccann · 2018-01-27T16:43:03Z

@etimberg can you clarify which value is null and how it is getting coalesced to 0 in https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.radialLinear.js#L446-L449 per your comment #5075 (comment)? I didn't quite follow what you were saying

@flaurida I'd like to see you not get blocked on this PR. What do you think about taking out the change to make fontColor support and array and send that as a new PR so that we can make progress on the two pieces separately and not have them blocked on each other?

etimberg · 2018-01-27T22:16:20Z

Sure, here's the current flow:

It starts in the radar controller where the scale is asked for the location of a value line 81
That gets into the radial linear scale in getPointPositionForValue
The actual transformation happens inside getDistanceFromCenterForValue

So, I think the solution would be to not call getPointPositionForValue function if the value is null, then set skip when updating the point model when the value is null. Line controller example

flaurida · 2018-02-01T22:20:07Z

Hi everyone, thanks so much for following up! From what I'm seeing, the PR should be broken up. To make sure we are on the same page, is it possible now to do either/both of the features 1) span gaps in radar charts 2) support multiple font colors for labels? If so, I will start working on a new PR for either/both. Thanks again 🎉

benmccann · 2018-02-02T04:12:50Z

For 1) Evert recommended a solution in his last comment - #5075 (comment)

For 2) I would remove it from this PR and make this PR focus just for spanning gaps. I recommend joining the Slack channel and asking Simon about the approach to take for multiple font colors and making that a separate PR. It sounds like he prefers we use scriptable options based on the discussion on the other PR. He knows the most about that. I haven't used scriptable options yet, so wouldn't be as much help there

simonbrunel · 2018-02-02T08:56:15Z

About point 2, we don't need to make that option (fontColor) scriptable right now because it involves a context and may make that task way more complex. However, we can make this option indexable as implemented by @flaurida. Though, that will be the only one for this scale but could be extended to other styling options later.

@benmccann It's different from #5153 because this PR doesn't try to set a different color for each text line (for which I would not recommend to use array) but for each label. Still, scriptable options are way more powerful and flexible than indexable (maybe we should consider to deprecate indexable and remove in v3?)

flaurida · 2018-02-06T14:22:50Z

Hi all,

Just tried to join the Slack channel but it says I need an invitation. So it seems like the way I implemented it is OK then? Will it be possible to write a test as well? I remember having a few issues there the last go around.

Sincerely,
Laura

benmccann · 2018-02-06T17:00:28Z

You can join the Slack channel here: https://chartjs-slack.herokuapp.com/

benmccann · 2018-02-06T17:02:43Z

docs/axes/radial/linear.md

@@ -104,7 +104,7 @@ The following options are used to configure the point labels that are shown on t
 | Name | Type | Default | Description
 | -----| ---- | --------| -----------
 | `callback` | `Function` | | Callback function to transform data labels to point labels. The default implementation simply returns the current string.
-| `fontColor` | `Color` | `'#666'` | Font color for point labels.
+| `fontColor` | `Color/Color[]` | `'#666'` | Font color for point labels.


you should remove this since it's being added in #5240

benmccann · 2018-04-22T16:56:01Z

I'll close this PR for now since it's being attempted in #5359. We can reopen if that one doesn't go through and we want to continue with this one

Allow spanning gaps for radar

362470c

flaurida changed the title ~~Allow spanning gaps for radar charts~~ Allow spanning data gaps for radar charts Dec 22, 2017

Fix options not being defined

90c2742

etimberg reviewed Dec 22, 2017

View reviewed changes

flaurida added 2 commits December 22, 2017 15:40

Support multiple point label font colors

85ac7e7

Update documentation

9f769b4

etimberg reviewed Dec 22, 2017

View reviewed changes

flaurida added 3 commits December 22, 2017 15:51

Add test for spanGaps in radar chart

5ad2ba9

Remove type in linear docs

0f859e7

Fix typo in test

3084a9e

Fix another typo in test

2df76e4

Try to address failing test

4bea405

simonbrunel requested changes Dec 23, 2017

View reviewed changes

flaurida added 2 commits December 23, 2017 11:49

Address review comment

23468aa

Remove unnecessary line

2a6b81c

Fix wrong deleted line

b28d11e

Add JSON file and image tests

936c822

Remove debug true from disable test

374e220

simonbrunel reviewed Dec 28, 2017

View reviewed changes

Fix issue with span gaps option

88ec1f2

flaurida mentioned this pull request Feb 6, 2018

Support multiple font colors for radial chart labels #5240

Merged

benmccann reviewed Feb 6, 2018

View reviewed changes

benmccann closed this Apr 22, 2018

nagix mentioned this pull request May 22, 2019

Support spanGaps in radar charts #6289

Merged

Allow spanning data gaps for radar charts #5075

Allow spanning data gaps for radar charts #5075

Conversation

flaurida commented Dec 22, 2017 • edited by simonbrunel

etimberg left a comment

Choose a reason for hiding this comment

etimberg Dec 22, 2017

Choose a reason for hiding this comment

flaurida Dec 22, 2017

Choose a reason for hiding this comment

flaurida commented Dec 22, 2017

etimberg commented Dec 22, 2017

flaurida commented Dec 22, 2017

flaurida commented Dec 22, 2017

etimberg commented Dec 22, 2017

flaurida commented Dec 22, 2017

flaurida commented Dec 22, 2017

etimberg commented Dec 22, 2017

simonbrunel left a comment

Choose a reason for hiding this comment

simonbrunel Dec 23, 2017

Choose a reason for hiding this comment

flaurida Dec 23, 2017

Choose a reason for hiding this comment

simonbrunel commented Dec 23, 2017

flaurida commented Dec 23, 2017

etimberg commented Dec 23, 2017

flaurida commented Dec 23, 2017

simonbrunel commented Dec 24, 2017

flaurida commented Dec 26, 2017

flaurida commented Dec 26, 2017

flaurida commented Dec 26, 2017

simonbrunel Dec 26, 2017

Choose a reason for hiding this comment

flaurida commented Dec 28, 2017

simonbrunel commented Dec 29, 2017

etimberg commented Dec 29, 2017

flaurida commented Dec 29, 2017

etimberg commented Dec 29, 2017

simonbrunel commented Dec 29, 2017

etimberg commented Dec 29, 2017

flaurida commented Dec 30, 2017

flaurida commented Jan 8, 2018

flaurida commented Jan 16, 2018

simonbrunel commented Jan 16, 2018

flaurida commented Jan 16, 2018

benmccann commented Jan 27, 2018 • edited

etimberg commented Jan 27, 2018

flaurida commented Feb 1, 2018

benmccann commented Feb 2, 2018

simonbrunel commented Feb 2, 2018

flaurida commented Feb 6, 2018

benmccann commented Feb 6, 2018

benmccann Feb 6, 2018

Choose a reason for hiding this comment

benmccann commented Apr 22, 2018

flaurida commented Dec 22, 2017 •

edited by simonbrunel

benmccann commented Jan 27, 2018 •

edited