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

Supply docs for barThickness #5854

Merged
merged 5 commits into from
Nov 28, 2018
Merged

Supply docs for barThickness #5854

merged 5 commits into from
Nov 28, 2018

Conversation

jedrekdomanski
Copy link
Contributor

@jedrekdomanski jedrekdomanski commented Nov 21, 2018

Add docs for barThickness option in Bar chart

Fixes #5853

@@ -139,6 +139,16 @@ The bar chart defines the following configuration options. These options are mer
| `gridLines.offsetGridLines` | `Boolean` | `true` | If true, the bars for a particular data point fall between the grid lines. The grid line will move to the left by one half of the tick interval. If false, the grid line will go right down the middle of the bars. [more...](#offsetgridlines)

### barThickness
This option needs to be configured on x axis in scales property.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to make this align with the docs for offsetGridLines. That means both that we should match the language (i.e. "This setting applies to the axis configuration.") and indentation (i.e. use 4-space indenting for the code sample)

@simonbrunel
Copy link
Member

@jedrekdomanski I'm pretty sure that all options listed in the "Common Configuration" belong to the scale options, meaning that the following docs is wrong:

These options are merged with the global chart configuration options, Chart.defaults.global, to form the options passed to the chart.

@etimberg @benmccann can you confirm?

If so, instead of duplicating "This setting applies to the axis configuration", I would rename the parent section from "Common Configuration" to "Scale Configuration" and reword the introduction paragraph for something like:

## Configuration Options

The bar chart accepts the following configuration from the associated **scale options**:

// ... the options table ...

For example:

options = {
    scales: {
        xAxes: [{
            barPercentage: 0.5,
            barThickness: 6,
            minBarLength: 2,
            gridLines: {
                offsetGridLines: true
            }
        }]
    }
}

What do you think?

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 22, 2018
@jedrekdomanski
Copy link
Contributor Author

If this is the case then yes, we should change that, although I am not an expert of Chart.js so I don't feel in a position to verify that. @etimberg @benmccann Can you confirm it?

@simonbrunel
Copy link
Member

@etimberg
Copy link
Member

I believe these are all scale options, but I only skimmed the code

@simonbrunel
Copy link
Member

@jedrekdomanski I think you can go ahead and fix the whole section instead.

- barThickness and other options no longer belong to `global`
options, they now belong to `scale` options
- example of usage of options provided in the table in the description
@simonbrunel
Copy link
Member

@jedrekdomanski should we also remove the similar part from offsetGridLines (line 167) since it's now redundant with the new doc?

This setting applies to the axis configuration. ... + snippet

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.

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.

4 participants