Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for gridLines/angleLines borderDash for polarArea/radar charts #5850

Merged
merged 1 commit into from
Nov 26, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Nov 19, 2018

This PR adds support for gridLines/angleLines borderDash and borderDashOffset options for polarArea and radar charts.

See https://jsfiddle.net/nagix/9nL5qcuy/.
screenshot 2018-11-20 at 1 14 22 am

Fixes #4976

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks really good, thank you @nagix

A few questions about the API:

  1. are we sure that borderDash* are part of the global defaults (can't find code / doc)?
  2. if so, should we fallback to these global values by "default" (meaning that angleLines.borderDash* should be undefined and defaults.global.borderDash* have values)?
  3. should we also fallback to the gridLines values? (ie. angleLines.borderDash* else gridLines.borderDash* else globalDefaults.borderDash*)

@nagix
Copy link
Contributor Author

nagix commented Nov 20, 2018

I just copied code from core.scale.js, but as you say, there's no borderDash* in the global defaults, and they don't work because the scale options have default values.

Why don't we introduce the global defaults for scale options like defaults.global.scale.{gridLines|scaleLabel|pointLabels|ticks}?
(it seems natural to me that angleLines.* fallback to the global gridLines.*)

I can open a new PR for this while removing globalDefaults from this PR.

@simonbrunel
Copy link
Member

Chart.defaults.scale.* are already the global defaults for all scales (merged here) so I don't think we should add another level of defaults options. Currently, gridLine.borderDash* options are resolved in this order:

  • scale.gridLine.borderDash* (ie. user options: chart.options.scales.{xAxes|yAxes}[i].*)
  • Chart.defaults.scale.gridLine.borderDash*
  • Chart.defaults.global.borderDash*

Looking how element options are resolved, they don't fallback to Chart.defaults.global.borderDash* so I think the existing behavior for scale is wrong, gridLine.borderDash* should be the top level. So I think we could change this line to:

borderDash = gridLines.borderDash || [];
borderDashOffset = gridLines.borderDashOffset || 0;

and resolve angleLines.* to:

borderDash = helpers.valueOrDefault(angleLineOpts.borderDash, gridLineOpts.borderDash) || [];
borderDashOffset = helpers.valueOrDefault(angleLineOpts.borderDashOffset, gridLineOpts.borderDashOffset) || 0;

I don't know if removing globalDefaults.borderDash* from the equation could break existing projects.

@nagix
Copy link
Contributor Author

nagix commented Nov 20, 2018

@simonbrunel I didn't notice that as Chart.defaults.scale.* is not documented (shouldn't it be?). I agree your suggested changes. I don't think removing globalDefaults.borderDash* causes any problem because it makes difference only when Chart.defaults.scale.borderDash* is manually set to false/undefined and Chart.defaults.global.borderDash* is defined.

@nagix nagix force-pushed the issue-5850 branch 3 times, most recently from a002da9 to b2f7e13 Compare November 20, 2018 15:06
@simonbrunel
Copy link
Member

I didn't notice that as Chart.defaults.scale.* is not documented (shouldn't it be?)

I agree, it should be better documented (the whole scale documentation would need a refresh actually).

I also agree that breaking cases are very unlikely.

@nagix
Copy link
Contributor Author

nagix commented Nov 22, 2018

As I have added checks for lineWidth and color, I also added a test for them.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nagix

@etimberg can you please review this one and give your thought about considering a color or width option with undefined or null final value (i.e. after options are merged) as "none". In the context of this PR, that means if (grid|angle)Lines.color and/or (grid|angle)Lines.lineWidth are undefined or null, we skip drawing grid/angle lines instead of drawing with the current context values (see this comment).

@chartjs chartjs deleted a comment from igi80 Nov 23, 2018
@etimberg
Copy link
Member

I'm ok with null and undefined being interpreted as no width. They'd cast to 0 which is what we're trying to avoid

@simonbrunel simonbrunel merged commit 0351a88 into chartjs:master Nov 26, 2018
@nagix nagix deleted the issue-5850 branch November 28, 2018 01:58
@kanaxz
Copy link

kanaxz commented Feb 28, 2019

How can I add this feature to my Chart.js file ?

@benmccann
Copy link
Contributor

See the documentation changes in this PR for info on how to use the feature. It is not yet available however. It will be part of Chart.js 2.8.0 which is not released yet. Hopefully it will be in the next couple weeks. We are working on getting it ready for release now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants