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

Creation of mixed charts should be possible also in version 2.0.0 #133

Closed
BorisGaliev opened this issue Jun 19, 2020 · 4 comments
Closed
Labels
bug Something isn't working preview Issues related to preview releases (upcoming versions)
Milestone

Comments

@BorisGaliev
Copy link

Describe the bug

It should be possible to create mixed charts that are a combination of two or more different chart types. E.g. see a code, which works with ChartJs.Blazor version 1.1.0, below

Which Blazor project type is your bug related to?

  • Client-Side
  • Server-Side

Code example

Please provide full code examples below where possible to make it easier for the developers to check your issues.

@using System.Drawing
@using ChartJs.Blazor.ChartJS.BarChart
@using ChartJs.Blazor.ChartJS.BarChart.Axes
@using ChartJs.Blazor.ChartJS.Common.Axes
@using ChartJs.Blazor.ChartJS.Common.Axes.Ticks
@using ChartJs.Blazor.ChartJS.Common.Enums
@using ChartJs.Blazor.ChartJS.Common.Properties
@using ChartJs.Blazor.ChartJS.Common.Wrappers
@using ChartJs.Blazor.ChartJS.LineChart
@using ChartJs.Blazor.Charts
@using ChartJs.Blazor.Util

<h2>Simple Bar Chart</h2>
<div class="row">
    <button class="btn btn-primary" @onclick="AddData">
        Add Data
    </button>
    <button class="btn btn-primary" @onclick="RemoveData">
        Remove Data
    </button>
</div>
<ChartJsBarChart @ref="_barChart"
                 Config="@_barChartConfig"
                 Width="600"
                 Height="300" />

@code
{

    private BarConfig _barChartConfig;
    private ChartJsBarChart _barChart;
    private BarDataset<DoubleWrapper> _barDataSet;

    protected override void OnInitialized()
    {
        _barChartConfig = new BarConfig
        {
            Options = new BarOptions
            {
                Title = new OptionsTitle
                {
                    Display = true,
                    Text = "Simple Bar Chart"
                },
                Scales = new BarScales
                {
                    XAxes = new List<CartesianAxis>
                    {
                        new BarCategoryAxis
                        {
                            BarPercentage = 0.5,
                            BarThickness = BarThickness.Flex
                        }
                    },
                    YAxes = new List<CartesianAxis>
                    {
                        new BarLinearCartesianAxis
                        {
                            Ticks = new LinearCartesianTicks
                            {
                                BeginAtZero = true
                            }
                        }
                    }
                }
            }
        };

        _barChartConfig.Data.Labels.AddRange(new[] { "A", "B", "C", "D" });

        _barDataSet = new BarDataset<DoubleWrapper>
        {
            Label = "My double dataset",
            BackgroundColor = new[] { ColorUtil.RandomColorString(), ColorUtil.RandomColorString(), ColorUtil.RandomColorString(), ColorUtil.RandomColorString() },
            BorderWidth = 0,
            HoverBackgroundColor = ColorUtil.RandomColorString(),
            HoverBorderColor = ColorUtil.RandomColorString(),
            HoverBorderWidth = 1,
            BorderColor = "#ffffff"
        };

        _barDataSet.AddRange(new double[] { 8, 5, 3, 7 }.Wrap());
        _barChartConfig.Data.Datasets.Add(_barDataSet);

        var lineSet = new LineDataset<ChartJs.Blazor.ChartJS.Common.Point>
        {
            Label = "Line",
            Hidden = false,
            BackgroundColor = ColorUtil.FromDrawingColor(Color.DimGray),
            BorderColor = ColorUtil.FromDrawingColor(Color.DimGray),
            BorderCapStyle = BorderCapStyle.Square,
            BorderJoinStyle = BorderJoinStyle.Bevel,
            Fill = false,
            PointBackgroundColor = ColorUtil.FromDrawingColor(Color.Black),
            BorderWidth = 1,
            PointBorderWidth = 1,
            PointRadius = 3,
            SteppedLine = SteppedLine.False
        };

        lineSet.AddRange(new List<ChartJs.Blazor.ChartJS.Common.Point> ()
        {
            new ChartJs.Blazor.ChartJS.Common.Point(0, 7),
            new ChartJs.Blazor.ChartJS.Common.Point(1, 3),
            new ChartJs.Blazor.ChartJS.Common.Point(2, 2),
            new ChartJs.Blazor.ChartJS.Common.Point(3, 5),

        } );

        _barChartConfig.Data.Datasets.Add(lineSet);
    }

    private async Task AddData()
    {
        var nowSecond = DateTime.Now.Second;
        _barChartConfig.Data.Labels.Add(nowSecond.ToString());
        _barDataSet.Add(new DoubleWrapper(nowSecond));

        await _barChart.Update();
    }

    private async Task RemoveData()
    {
        if (_barChartConfig.Data.Labels.Count > 0)
        {
            _barChartConfig.Data.Labels.RemoveAt(_barChartConfig.Data.Labels.Count - 1);
        }

        if (_barDataSet.Data.Count > 0)
        {
            _barDataSet.RemoveAt(_barDataSet.Data.Count - 1);
        }

        await _barChart.Update();
    }
}
@BorisGaliev BorisGaliev added the bug Something isn't working label Jun 19, 2020
@Joelius300 Joelius300 added the preview Issues related to preview releases (upcoming versions) label Jun 19, 2020
@Joelius300 Joelius300 added this to the 2.0.0 milestone Jun 19, 2020
@Joelius300
Copy link
Contributor

Joelius300 commented Jun 21, 2020

There are multiple solutions to this with both pros and cons.

I'm not listing You can mix any dataset into any chart as a pro because that's a requirement if I understand the chart-mixing correctly. I didn't have enough knowledge on Chart.js mixing functionality and focused a bit much on typesafety when implementing the dataset-collections for #96. I still like the system I put in place for its typesafety but obviously it doesn't suit our needs and has to be changed. This only affects the dataset-collections, not the datasets themselfs.

The simple approach

Instead of having the dataset-collections, we could just have a List<IDataset>.

Pros

  • Very simple to use (it's a basic list, what can you say)
  • No additional methods or classes like DatasetCollection
  • Very easy to implement
  • Can (and maybe should) be hidden behind an interface like IList<IDataset>

Cons

  • Not very typesafe1
  • Potentially causes confusion because a lot more is allowed than new users might expect
  • No guidance for which type to chose (apart from common sense and docs 😉)

The more complex and explicit approach

This approach still uses dataset-collections and they would be implemented similarly but even more restrictive. The dataset-collection would provide methods to add only supported datasets for the same chart-type. So the BarDatasetCollection would allow adding BarDataset<TimePoint>, BarDataset<FloatingBarPoint>, BarDataset<int>, BarDataset<long> and BarDataset<double>. We would again use the trick of Add overloads in order to allow for a typesafe collection initializer.

Then in addition to the "supported" datasets, you could also add any other IDataset via a different method called something like AddMixingDataset.

Pros

  • Very typesafe
  • You're automatically guided to the "correct" (definitely supported) dataset types
  • You have to be explicit if you want to break the restriction

Cons

  • A bit more verbose on the clients side
  • Can't really be hidden behind an interface
  • Lots of redundant code on the libraries side (code generation with T4 or other tools would be used)
  • A lot harder to implement than a simple list

What do you think? Which solution do you think is more enjoyable to use?

My vote is for the simple solution. It's less work, less complexity and the complex solution doesn't seems to provide enough value to be worth the cons and especially the work.


1 When thinking about this point you should consider that the basic pattern is to store a reference to your dataset and modify it with that reference. You shouldn't need to query the datasets of a chart and thus typesafety looses importance.

@BorisGaliev
Copy link
Author

Because your library is based on chart js, it might make sense to provide a similar API: there it is implemented on a dataset level, so both your approaches correspond to their philosophy, and if the first one is the easier one, it would be just fine

@Joelius300
Copy link
Contributor

Agreed. One thing to consider is that the options can be mixed as well as far as I know. That shouldn't be possible for every chart though but we could create a new chart config which allows for more or less dynamic object that can be composed of several configs. Something like their helpers.merge but only for mixable chart configs.

Or we could leave it as is and let the user plug in their own implementation of a chart config which makes it so they can extend any config by deriving from it and then using that type in Chart. Easier for us, just as flexible for the user but requires a bit more code and there's less guidance. If we document that and provide a sample, I think this is the way to go.

@BorisGaliev
Copy link
Author

If an example, where it is explained how to derive and use inheritance, becomes available, it will be quite okay, I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Issues related to preview releases (upcoming versions)
Projects
None yet
Development

No branches or pull requests

2 participants