muqeet-khan / BlazorComponents

Simple reusable Blazor component library
50 stars 10 forks source link

Visual glitches when updating charts #13

Closed miroslavp closed 5 years ago

miroslavp commented 6 years ago

I noticed visual glitches on the chart when using the update button from the example. It turns out that the cause for that is because you haven't destroyed the old chart object before setting with new one.

In BlazorComponents.js, for every update function you should add if (myChartIndex !== -1) { myChart.chart.destroy(); } right before myChart.chart = {};

Another thing I noticed is that in your Initialize functions you create a new chart object even if you don't add it to the collection. So I think you can improve it by changing the code from this:

    let thisChart = initializeChartjsChart(data, 'bar');

    if (!BlazorCharts.find(currentChart => currentChart.id === data.canvasId))
        BlazorCharts.push({ id: data.canvasId, chart: thisChart });

to this

    if (!BlazorCharts.find(currentChart => currentChart.id === data.canvasId)) {
        let thisChart = initializeChartjsChart(data, 'bar');
        BlazorCharts.push({ id: data.canvasId, chart: thisChart });
    }
miroslavp commented 6 years ago

ChartJsBarChart.cshtml ChartJsLineChart.cshtml ChartJsRadarChart.cshtml should implement IDisposable and destroy the chart on Dispose

@implements IDisposable

public void Dispose()
{
    ChartJSInterop.DestroyChart(Chart.CanvasId);
}

Here's the method in the interop class

    public static Task<bool> DestroyChart(string canvasId)
    {
        return JSRuntime.Current.InvokeAsync<bool>("BlazorComponents.ChartJSInterop.DestroyChart", new[] { canvasId });
    }

Here's the js function

DestroyChart: function (canvasId) {
    if (!BlazorCharts.find(currentChart => currentChart.id === canvasId))
        throw `Could not find a chart with the given id. ${canvasId}`;

    let myChart = BlazorCharts.find(currentChart => currentChart.id === canvasId);

    let myChartIndex = BlazorCharts.findIndex(currentChart => currentChart.id === canvasId);

    if (myChartIndex !== -1) {
        myChart.chart.destroy();
        BlazorCharts.splice(myChartIndex, 1);
    }

    return true;
}

As you can see, we remove the chart object from the global array with this line when the component is destroyed BlazorCharts.splice(myChartIndex, 1);

miroslavp commented 6 years ago

Also update methods of ChartJsBarChart.cshtml ChartJsLineChart.cshtml ChartJsRadarChart.cshtml should be parameterless. Right now they look like this:

public void UpdateChart(ChartJSChart<ChartJsBarDataset> updatedChart)
{
    ChartJSInterop.UpdateBarChart(updatedChart);
}

However they should pass the local property instead like this:

public void UpdateChart()
{
    ChartJSInterop.UpdateBarChart(Chart);
}

The user should always modify the local property instead of passing new objects. This is important because otherwise he/she can pass a new object with new canvasId which would be wrong. Also I recommend you to remove the setter of the CanvasId property and pass the id in the constructor if you want to specify specific canvas id

public class ChartJSChart<T> where T: ChartJsDataset
{
    public ChartJSChart()
    {

    }

    public ChartJSChart(string canvasId)
    {
        this.CanvasId = canvasId;
    }

    public string ChartType { get; set; } = "bar";
    public ChartJsData<T> Data { get; set; }
    public ChartJsOptions Options { get; set; }
    public string CanvasId { get; } = $"BlazorChartJS_{new Random().Next(0, 1000000).ToString()}";
}
muqeet-khan commented 5 years ago

I am closing this now, as this should be fixed by release 0.4.0. If you still have issues let me know and I can reopen.