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

split radial scale lineArc setting into two settings #3782

Merged
merged 1 commit into from
Mar 3, 2017
Merged

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Jan 11, 2017

labelPoints controls the visibility of angle lines and point labels
gridLines.circular toggles how the radial lines are drawn. When false, straight lines are drawn. When true, circular lines are drawn.

Allows creating charts that look like:
circular radar

Resolves #3082

API Change

This is technically a breaking API change since the old setting, lineArc is no longer used. I don't know if this will cause problems since I think this setting is only used internally and it is not well documented.

@@ -310,14 +310,15 @@ The following additional configuration options are provided by the radial linear

Name | Type | Default | Description
--- | --- | --- | ---
lineArc | Boolean | false | If true, circular arcs are used else straight lines are used. The former is used by the polar area chart and the latter by the radar chart
labelPoints | Boolean | true | If true, angle lines and point labels are drawn.
Copy link
Member

Choose a reason for hiding this comment

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

Is it something equivalent to pointLabels.display && angleLines.display. If so, maybe it would be better to split this option in these two *.display for a finer control. Else, I don't think the option name is great because then this scale will have very similar option names: labelPoints and pointLabels, which is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's sort of like those. I'll update the code to use those two display settings instead

@etimberg
Copy link
Member Author

Made the changes suggested above. For now, I just use opts.angleLines.display since there is no opts.pointLabels.display setting. I can add the second setting if we want it. I think the logic would then change to be opts.angleLines.display || opts.pointLabels.display since we'd want the behaviour if there are no lines but there are labels and vice versa.

angleLines | Object | - | See the Angle Line Options section below for details.
pointLabels | Object | - | See the Point Label Options section below for details.
ticks | Object | - | See the Ticks table below for options.

#### Angle Line Options

The following options are used to configure angled lines that radiate from the center of the chart to the point labels. They can be found in the `angleLines` sub options. Note that these options only apply if `lineArc` is false.
The following options are used to configure angled lines that radiate from the center of the chart to the point labels. They can be found in the `angleLines` sub options. Note that these options only apply if `labelPoints` is true.
Copy link
Member

Choose a reason for hiding this comment

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

labelPoints doesn't exist anymore!

@@ -327,7 +327,7 @@ lineWidth | Number | 1 | Width of angled lines

#### Point Label Options

The following options are used to configure the point labels that are shown on the perimeter of the scale. They can be found in the `pointLabels` sub options. Note that these options only apply if `lineArc` is false.
The following options are used to configure the point labels that are shown on the perimeter of the scale. They can be found in the `pointLabels` sub options. Note that these options only apply if `labelPoints` is true.
Copy link
Member

Choose a reason for hiding this comment

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

labelPoints doesn't exist anymore!

@simonbrunel
Copy link
Member

User should be able to choose to display lines without labels and vice-versa, so angleLines.display and pointLabels.display should be independent, setting one should not force display the other one, meaning that if we introduce pointLabels.display later, it will be a breaking change! Or I misunderstood your point.

Else, if displaying labels depends on displaying lines, pointLabels should be a child of angleLines: angleLines.labels.display.

@etimberg
Copy link
Member Author

Agreed, I will add the pointLabels.display setting. I think users will want to do this independently of the lines so we should support it.

… new settings.

gridLines.circular is a new option that toggles circular lines. This allows radar charts with circular lines #3082
pointLabels.display is a new option that toggles the display of point labels.
The existing angleLines.display is used with the new pointLabels.display setting is used to trigger the radar like settings.
This required changing the default polar area config.
@etimberg
Copy link
Member Author

Added the pointLabel.display setting and used it accordingly. Removed the 2 spurious references to labelPoints in the docs

@etimberg etimberg added this to the Version 2.6 milestone Feb 11, 2017
@etimberg
Copy link
Member Author

@simonbrunel when you get a chance, can you review again?

@simonbrunel
Copy link
Member

Looks good, I would also add support for angleLines: false and pointLabels: false as shortcut for both *.display: false (as we have for legend / title).

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.

2 participants