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

chartjs-2.0 Hardcoded property "fill" in getDataSets #309

Closed
courchef opened this issue Feb 19, 2016 · 5 comments
Closed

chartjs-2.0 Hardcoded property "fill" in getDataSets #309

courchef opened this issue Feb 19, 2016 · 5 comments
Labels

Comments

@courchef
Copy link

The property fill is hardcoded to true in function getDataSets

    function getDataSets (labels, data, series, colors) {
      return {
        labels: labels,
        datasets: data.map(function (item, i) {
          return angular.extend({}, colors[i], {
            label: series[i],
            data: item,
            fill: true
          });
        })
      };
    }

Is it possible to convert this into an option?

@courchef
Copy link
Author

In my cshtml file, I added a tag named chart-fill

<canvas id="primary" class="chart chart-line" chart-fill="false"></canvas>

In the ChartJsFactory, I added the property in the scope:

        scope: {
          getColour: '=?',
          chartType: '=',
          chartData: '=?',
          chartLabels: '=?',
          chartOptions: '=?',
          chartSeries: '=?',
          chartColors: '=?',
          chartFill: '=',
          chartLocale: '=?',
          chartClick: '=?',
          chartHover: '=?'
        },

In method getDataSets and getData, I added a fill parameter instead of the hardcoded value:

    function getDataSets (labels, data, series, colors, fill) {
      return {
        labels: labels,
        datasets: data.map(function (item, i) {
          return angular.extend({}, colors[i], {
            label: series[i],
            data: item,
            fill: fill
          });
        })
      };
    }

And finally, when calling getDataSets or getData, I add the scope property chartFill:

            var data = Array.isArray(scope.chartData[0]) ?
              getDataSets(scope.chartLabels, scope.chartData, scope.chartSeries || [], scope.chartColors, scope.chartFill) :
              getData(scope.chartLabels, scope.chartData, scope.chartColors, scope.chartFill);

What do you think?

@jtblin jtblin added the bug label Feb 20, 2016
jtblin added a commit that referenced this issue Feb 20, 2016
`Fill` option can be set via Chart.js options and already default to `true`: https://github.com/nnnick/Chart.js/blob/v2.0-dev/docs/00-Getting-Started.md
@jtblin
Copy link
Owner

jtblin commented Feb 20, 2016

Thanks. Agreed it should not be hardcoded. Looking at the 2.0 docs, it is possible to set the value via standard Chart.js options and it already defaults to true so I've just removed it.

https://github.com/nnnick/Chart.js/blob/v2.0-dev/docs/00-Getting-Started.md

@jtblin jtblin closed this as completed Feb 20, 2016
@courchef
Copy link
Author

I removed all my modifications, updated to your last commit and set fill: false in default options of Chart.js

Many thanks for your work!

@jtblin
Copy link
Owner

jtblin commented Feb 20, 2016

👍

@seralf
Copy link

seralf commented Sep 17, 2016

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

No branches or pull requests

3 participants