amcharts / editor4

amCharts Chart Editor based on amCharts 4
Other
27 stars 36 forks source link

EditorLauncher should include Chart Type in save event #59

Closed ChazUK closed 3 years ago

ChazUK commented 3 years ago

Rendering a chart using am4core.createFromConfig requires a type to be provided, which at the moment is not being sent in the event object from EditorLauncher.

ailon commented 3 years ago

The type parameter in the am4core.createFromConfig is optional. Editor puts the type in the config itself so it's not needed in the am4core.createFromConfig call.

ailon commented 3 years ago

Were you able to figure it out? I got a notification of a followup message from you but I can't see it on GH :)

ChazUK commented 3 years ago

I can see that when trying to save the event into localStorage or do any manipulation with it the type property is removed from the object.

  renderChart(event) {
    console.log(JSON.stringify(event));
    if (this.launcher) {
      this.launcher.close();
    }

    if (event.appliedThemes) {
      this.applyChartThemes(event.appliedThemes);
    }

    if (this.chart !== undefined) {
      this.chart.dispose();
    }

    this.chart = am4core.createFromConfig(
      event.chartConfig,
      document.getElementById("chartdiv")
    );

    const charts = JSON.parse(localStorage.getItem("charts"));
    charts.push(event);
    console.log(charts);
    localStorage.setItem("charts", JSON.stringify(charts));
    console.log(localStorage.getItem("charts"));
  }

The initial stringify of the event "chartConfig":{"type":"PieChart3D" ... then adding that to an empty array removes the type property.

ailon commented 3 years ago

Hard to say. Could be that type is some sort of reserved keyword somewhere and gets removed/swallowed/etc. But there's nothing we can do about it on the editor's side as it is what is needed for the charts.

A quick workaround would be to save that type separately and then pass it to createFromConfig as a parameter.

ailon commented 3 years ago

Having said that "type" is used throughout the JSON config on lower levels for all kinds of objects. So if those are removed as well, the above workaround won't work.

ChazUK commented 3 years ago

I think it's something to do with the object itself, possibly omitting it from the model. Here you can see I've printed, and the second console log object doesn't contain type property.

    console.log(JSON.stringify(event.chartConfig));
    console.log(event.chartConfig);
    console.log("----");
Screenshot 2021-07-14 at 11 29 16

Then after just running a console.log on a copy pasted version of the object displays it displays type.

Screenshot 2021-07-14 at 11 29 45
ailon commented 3 years ago

Not sure I understand. I see type in both of your screenshots. What am I missing?

ChazUK commented 3 years ago

Maybe this will show it better, even though type is listed on the top row, it seems to be a hidden or private property on the model that is not being copied properly.

typemissing

ailon commented 3 years ago

But the "unexpanded" first line has type. And if you console.log(event.chartConfig.type) I'm almost certain you'll get it. So I'm not sure if we are debugging browser devtools here or is there a real issue you are hitting? :)

ailon commented 3 years ago

Btw, you shouldn't pass the actual config object to createFromConfig() if you plan to reuse it later. The charts, unfortunately, modify the config in the process. You'd need to create a copy before passing it if you want to retain the "original".

ChazUK commented 3 years ago

Ok, can you try

import cloneDeep from 'lodash/cloneDeep';
...

const config = cloneDeep(event);
console.log(event);
console.log(config);

and see that type is being removed from the chartConfig object

Screenshot 2021-07-14 at 12 38 44
ailon commented 3 years ago

What are you passing to the createFromConfig() later on?

ChazUK commented 3 years ago

As pasted above I'm storing the event response object in an array, then converting to JSON string to store in localStorage.

Then on the EditChart.js page I am taking the string from localStorage, parsing the JSON and then looping through the array, passing chart.chartConfig to the createFromConfig method.

componentDidMount() {
    this.charts = JSON.parse(localStorage.getItem("charts"));
    this.charts.map((chart, index) => this.renderChart(index, chart));
}

renderChart(index, chart, appliedThemes) {
    if (this.launcher) {
        this.launcher.close();
    }

    if (appliedThemes) {
        this.applyChartThemes(appliedThemes);
    }

    if (this.chart[index] !== undefined) {
        this.chart[index].dispose();
    }

    this.chart[index] = am4core.createFromConfig(
        chart.chartConfig,
        document.getElementById(`chartdiv${index}`)
    );
}
ailon commented 3 years ago

OK, I've tried to do a simple demo mod to store the config in localStorage on the new chart page and then get it on the edit chart page. Here it is https://codesandbox.io/s/amcharts-editor-localstorage-sample-b5qe6

Seems to be working for me. The only two changes from the demo bundled with the Editor are:

Line 50 in NewChart.js:

localStorage.setItem("chartConfig", JSON.stringify(event.chartConfig));

Line 135 in EditChart.js:

chartConfig = JSON.parse(localStorage.getItem("chartConfig"));

I still think that the issue in your case is that you are passing the config to charts at some point before you save it or after you retrieve it and that causes the config to get modified. Hard to say without seeing a [non]working example.

ChazUK commented 3 years ago

https://codesandbox.io/s/amcharts-editor-localstorage-sample-forked-d474y?file=/src/NewChart.js line 45 - 51

  1. Click New Chart
  2. Click the box to open amcharts editor
  3. Select 3D Donut Chart
  4. Click Save
  5. Compare console log for event, cloneDeep and JSON steps . type is missing from cloneDeep copy.

JSON.parse(JSON.stringify()) is a costly way of copying an object. https://stackoverflow.com/questions/12690107/clone-object-without-reference-javascript

ailon commented 3 years ago

I'm not sure if I want to get into debugging cloneDeep or other things here. Maybe it is treating "type" in some special way or anything. I'm not willing to go deep investigating it, sorry.

JSON.stringify() maybe costly if you are doing it in a big loop or something but here it just works, doesn't require 3rd party dependencies, and the "costs" are negligible, imho.

Also, I think your initial goal was to save a string JSON anyway so you'd be using it in any case on top of cloneDeep.

ChazUK commented 3 years ago

Ok, I've never seen this issue happen with cloneDeep before. Hopefully no one else will come across this issue.