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 directive chart-dataset-overload #391

Closed
wants to merge 1 commit into from

Conversation

MDO-OTB
Copy link

@MDO-OTB MDO-OTB commented May 17, 2016

We could not set the options included in the chart dataset. We could only set the global options thanks to the directive chart-options. Therfore we could not set an option per chart like described in Issue #370

I added a directive chart-dataset-overload. If it is present, createChart will merge both the dataset and the data set overload at the final step of the dataset creation.

That's my first pull request with github !

We could not set the options included in the chart dataset. We could only set the global options thanks to the directive chart-options. Therfore we could not set an option  per chart like described in Issue jtblin#370 .
I added a directive chart-dataset-overload. If it is present, createChart will merge both the dataset and the data set overload at the final step of the dataset creation.
@nirre7
Copy link

nirre7 commented May 22, 2016

This is great and gives flexibility how we can create charts.

I was just about to add an issue regarding how one would be able to set colors for the bars in the bar chart based on values for of the dataset(s). This pull request should fix that since I can now create and array with colors based on data and then use the overload feature.
http://www.chartjs.org/docs/#bar-chart-data-structure

Would be awesome if this could be merged asap.

Thanks in advance
Niclas

@jtblin
Copy link
Owner

jtblin commented May 22, 2016

Thanks for the PR @MDO-OTB, do you have examples of options that you couldn't set using the standard options?

@nirre7, not sure I understand why you cannot do this currently? Can't you just pass an array of colors to chart-colors?

@MDO-OTB
Copy link
Author

MDO-OTB commented May 22, 2016

Hello,

No i don't have example. I use only chart-dataset-overload with the options which must be different between curves.

For example :
<canvas id="graphLink{{$index}}" class="chart chart-line" chart-data="data[$index]" chart-options="options" chart-labels="labels[$index]" chart-series="series" chart-colors="colors" chart-dataset-overload="mydataset"></canvas>

option available for each curve are there :

    $scope.options = {
        responsive: true,
        legend : {
            display: true,
            position: "bottom",
            labels: {
                fontColor: "#000"
            }
        },
        tooltips: {
            enabled:false
        },
        animation : {
            duration: 0
        },
        scales: {
            yAxes: [{
                type: "linear",
                position: "right",
                ticks: {
                    fontSize:12,
                    min: 0,
                    max: 100,
                    stepSize: 10
                }
            }],
            xAxes: [{
                position: "bottom",
                ticks: {
                    fontSize:12,
                    autoSkip: false,
                    callback: ACallback
                }
            }]
        },
        elements: {
           point: {
            radius: 0
        },
    }
};

with a $index which belong to [0;3], options which are not identical for each curve are set with chart-dataset-overload :

    $scope.mydataset = [
        {
            fill: false,
            borderWidth:2.5
        },
        {
            fill: false,
            borderWidth:1.5
        },
        {
            fill: true,
            borderWidth:0.5
        },
        {
            fill: true,
            borderWidth:3.5
        }
    ]

Personally , I think this directive can be helpfull because angular-chart is a wrapper for chartjs. So, we can provide new ways to configure charts (char-colors, chart-data, chart-labels) and keep the native way thanks chart-chart-dataset-overload.

It's as you want :-) and in all cases thank you for this project

@jtblin
Copy link
Owner

jtblin commented May 22, 2016

Thanks, makes sense and sounds like a good idea. A few things I'm wondering:

  • would that makes sense to overwrite individual options for the other types of chart as well e.g. pie, doughnut, etc.?
  • shall we call the attribute datasetOptions or optionsOverride instead?
  • could we set the y-axis using the same structure instead of requiring a different attribute for y-axis?

Example for the later, I could specify the y-axis using

    $scope.mydataset = [
        (..)
        {
            fill: false,
            borderWidth:2.5,
            yAxisID: 'y-axis-2' // the axis would still be set via options as of today e.g. https://github.com/jtblin/angular-chart.js/blob/chartjs-2.0/examples/app.js#L41
        },
        (..)
    ]

Given the scope of changes, would you mind also do the following?

Thank you

@@ -283,6 +285,9 @@
if (yaxis) {
dataset.yAxisID = yaxis[i];
}
if (datasetOverload) {
angular.merge(dataset,datasetOverload[i])
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: space after comma.

@nirre7
Copy link

nirre7 commented May 23, 2016

@jtblin In most cases the current api is great.
However we need to be able to set color based on the value in the dataset(s), we weren't able to do it with the current attributes. With the overload attribute we can set "all" attributes we need to set per for example a bar in a bar chart.

Albeit this could be considered "advance" usage.

Here is an example:

        var setColorBasedOnData = function (data) {
            return data.map(function (value) {
                return value > 0 ? '#2ecc71' : '#ee3226'; // green and red
            });
        };

        $scope.dataAndConfig = [
            {
                label: "Override Series A",
                backgroundColor: setColorBasedOnData([65, -59, 80, 81, -56, 55, -40]),
                borderWidth: 1,
                data: [65, -59, 80, 81, -56, 55, -40]
            },
            {
                label: "Override Series B",
                backgroundColor: setColorBasedOnData([28, 48, -40, 19, 86, 27, 90]),
                borderColor: "rgba(255,99,132,1)",
                borderWidth: 3,
                hoverBackgroundColor: "rgba(255,99,132,0.4)",
                hoverBorderColor: "rgba(255,99,132,1)",
                data: [28, 48, -40, 19, 86, 27, 90]
            }
        ];

Regards
Niclas

@AYapejian
Copy link

@nirre7 Totally agree, that would be helpful imo

@jtblin
Copy link
Owner

jtblin commented Jun 19, 2016

@MDO-OTB I'm cherry-picking your commit and making a few changes to integrate that into the library. It should be available soon.

jtblin added a commit that referenced this pull request Jun 19, 2016
* rename datasetOverload to datasetOverride
* use datasetOverride for pie and doughnut charts as well
* add unit and integration tests
* add examples
* fix #370, #336, #391
jtblin added a commit that referenced this pull request Jun 19, 2016
* add attribute `chart-dataset-overload`
* complete support for `dataset-override`
* rename `datasetOverload` to `datasetOverride`
* use `datasetOverride` for pie and doughnut charts as well
* add unit and integration tests
* add examples
* fix #370, #336, #391
* remove `y-axis` attribute as it can now be implemented using the `dataset-override` attribute
* compile assets and minor fixes for jshint
@jtblin
Copy link
Owner

jtblin commented Jun 19, 2016

Now merged. Thanks a ton for the original implementation @MDO-OTB.

@nirre7 what you want to do cannot be implemented as-is for now, the data still needs to be passed separately. It was looking like it was going to make the process too complicated. However we should be close to what you wanted to do, maybe just have to pass the data separately.

@jtblin jtblin closed this Jun 19, 2016
@MDO-OTB
Copy link
Author

MDO-OTB commented Jun 19, 2016

Thank very much for the merge, it is very nice. I must say sorry for not answering earlier but I have been really busy working on other projets.

@nirre7
Copy link

nirre7 commented Jul 7, 2016

Great work, thank you all! Sorry for the late response ..
@jtblin yepp that is what I'm doing atm. Will look into if I can simplify "my workaround" now.

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

Successfully merging this pull request may close these issues.

5 participants