[Feature] FusionCharts manual chart config to be able to plot anything FusionCharts supports

zegerhoogeboom commented 3 years ago

Summary

Currently only a few chart options are supported (e.g. line/bar chart). The underlying library, FusionCharts, supports many more chart types and options. This feature request is to add a chart type "Manual" (in the "Chart type" dropdown) where the user supplied data is not expected to be in the format Array<x:string, y:number> but in the format Object, representing the full config for a FusionChart chart (maybe except for the renderAt property which AppSmith can fill in).

Motivation

The current charting options are useful but don't allow for configuring chart options beyond the explicit ones in the interface (e.g. x-axis label). If a user would like more complex charting options the user should be able to manually specify the config even though this takes more effort on the user side.

Additional Context

So, in this dropdown another option could be added: "Manual"

Additionally, a link somewhere to the FusionCharts documentation (https://www.fusioncharts.com/fusioncharts) would be useful when the "Manual" option is selected.

zegerhoogeboom commented 3 years ago

As a note, I would be willing to try to implement this myself if there is a consensus this feature is a good idea and has a chance to get merged when implemented.

areyabhishek commented 3 years ago

@zegerhoogeboom We'd love some help with this. We definitely need more charts. We can ask one of our designers+engineers to work with you on this to help navigate the codebase+get designs. What do you think?

@riodeuno @Nikhil-Nandagopal Thoughts on this?

zegerhoogeboom commented 3 years ago

@areyabhishek Thanks for the quick response, sounds good. Though as far as I can tell solving the problem in the way i described (simply adding a "Manual" option and having the user specify the full configuration) should be relatively easy.

I would have to make sure the chartConfig object in app/client/src/components/designSystems/appsmith/ChartComponent.tsx -> createGraph() is simply taking the configuration from the user, making sure to override the config with the width/height/renderAt something like

const chartConfig = {
  Object.assign(
    configFromUser,
    {width: "100%", height: "100%", renderAt: this.props.widgetId + "chart-container"}
}

and hide the other configuration options in the popup interface (as the user should specify them in the JSON config).

The main question/problem from my side which maybe you or @riodeuno @Nikhil-Nandagopal could have some input on is if you understand the solution I'm proposing and whether you agree with it. Of course it would be perfect if it would be possible to integrate all chart types and configuration options through the interface but doing that looks quite difficult. Hence, if it's at least possible to specify the chart config manually it opens the possibility to chart anything that FusionCharts can do even though the configuration is a bit more manual.

Nikhil-Nandagopal commented 3 years ago

@zegerhoogeboom the reason we enforce the Array<x, y> type is that we maintain a library agnostic interface so that in the future swapping out fusion charts for another library becomes seamless. I do see the value in a control like this so I would suggest

Add a new chart type 'Custom Fusion Chart'
Add an input called fusion chart config that shows up only for this chart type and hides the other default controls The rest of your solution looks good

riodeuno commented 3 years ago

@Nikhil-Nandagopal @zegerhoogeboom I agree with the solution as well. Having a library agnostic feature with all supported types of charts will be ideal, but this solution will unlock all available features in fusion charts, which can become useful sooner.

As for @Nikhil-Nandagopal 's point 2 -- We have a feature in the platform, currently in the works, which would facilitate the conditional hiding of controls.

zegerhoogeboom commented 3 years ago

Thanks for the input @Nikhil-Nandagopal and @riodeuno! I understand the enforcing of the Array<x, y> type and glad you see the value in being able to specify a config manually. I actually started trying to implement this yesterday and it's in a bit of a rough shape at the moment but this is the solution that you are referring to, right?

Could you point me to the place where this feature is being developed @riodeuno such that I can track it's development and merge it for my own use before it's released?

riodeuno commented 3 years ago

@zegerhoogeboom Yes, this is what @Nikhil-Nandagopal has outlined.

We're developing the property pane feature along side enhancements to the table widget. https://github.com/appsmithorg/appsmith/pull/1780

The API for the property pane configurations which can be used to hide controls, can be seen here: https://github.com/appsmithorg/appsmith/blob/f5e2a98e078b27a3279ecc04a14f1f22c665b5c4/app/client/src/constants/PropertyControlConstants.tsx#L37

It may not be trivial to extract or isolate the property pane updates from the changes in this PR.

zegerhoogeboom commented 3 years ago

Thanks @riodeuno, I see it's a pretty big PR that's been worked on for a while. Do you think it will get merged/released in the coming weeks? If so I can branch out #1780 and do my changes on top of that. If not I can try to isolate the property pane changes though it doesn't look trivial indeed.

riodeuno commented 3 years ago

@zegerhoogeboom We are in the last legs of the development process for this feature. I believe it will be merged into the release branch in the coming weeks. Instead of waiting for the release of this PR. I will isolate the property pane changes in a separate PR so that we can merge it into release sooner.

riodeuno commented 3 years ago

@zegerhoogeboom I've just finished extracting the property pane enhancements in a separate PR #2670

Once this is tested, we will be able to merge it into the release branch.

zegerhoogeboom commented 3 years ago

Wow that's awesome, thanks for that! I'll get to fully implementing this feature probably tomorrow :+1:

zegerhoogeboom commented 3 years ago

Hi guys. Would you be open to having an additional option 'Custom Plotly Chart'?

The custom Fusion Charts config is working which is great (along with the conditional hiding of the other property panel attributes), but I realized a bit late they don't support 3D plots (with 3 axes, not just e.g. bars drawn as 3D) which I really need for my use case. I am currently using 3D Plotly plots in clunky custom interfaces which I would really like to migrate into Appsmith.

See e.g. here for the type of plots I'm referring to: https://plotly.com/javascript/3d-surface-plots/

So, given a proper implementation, would you be open to me adding two chart options: 'Custom Fusion Chart' and 'Custom Plotly Chart'?

Nikhil-Nandagopal commented 3 years ago

@zegerhoogeboom that sounds fine to me. Could you raise 2 separate PRs for this?

Nikhil-Nandagopal commented 3 years ago

@riodeuno we should pick up the custom widget soon!

zegerhoogeboom commented 3 years ago

@Nikhil-Nandagopal Great, will do. I'm pretty much done with both widget types (and started using them in my company's Appsmith instance) but they're currently implemented in the same branch.

I'll separate them, make some documentation changes on the docs repository and see what else I have to prepare to submit the PR but you can expect them soon, at some point this week I think.

Nikhil-Nandagopal commented 3 years ago

@zegerhoogeboom 👋 are you having any trouble getting this PR merged in? Do you need any help?

zegerhoogeboom commented 3 years ago

Hi @Nikhil-Nandagopal thanks for checking in! I'm indeed having some trouble as I branched out of #2670 and the release branch is moving quite fast.

When I compare the current state of my branch against the release branch it says there's 66 files changed which is of course not that I changed 66 files but just because my branch is out of sync. Or at least - I'm probably also ahead because I branched out of #2670 but I don't exactly know what changes are legitimate from there and which "changes" are just because i'm out of sync. (from: https://github.com/appsmithorg/appsmith/compare/release...zegerhoogeboom:feature/custom-fusionchart-config)

I was thinking to maybe just start a fresh branch from the release branch and just cherry pick my commits on top and manually merge any conflicts as I don't really know how to resolve the differences that there currently are. Problem with that also is that #2670 is not merged in yet =/.

So yeah, could definitely use some help! I think any of these would be very helpful:

Somehow get my branch in sync with release again such that only my actual changes appear in the diff
If #2670 would be merged then the PR diff should show only my changes - that would be very good as from there I think I can figure it out
I can probably submit a PR to merge into #2670, that would be a lot easier, but I'm not sure that's any useful.

Would be great to get the custom fusion chart change merged indeed!

Nikhil-Nandagopal commented 3 years ago

@riodeuno can you please help here? :)

riodeuno commented 3 years ago

Hi @zegerhoogeboom! Let's go with step 3. A PR to merge into #2670. I can take care of any issues in merging into release from there.

We had some updates pending so #2670 hasn't been merged yet. There will be minor conflicts but it should be easier for me as I'm aware of the changes in both #2670 and #2816.

zegerhoogeboom commented 3 years ago

Awesome, that I can manage much more easily, will submit!

zegerhoogeboom commented 3 years ago

Hi @riodeuno I just submitted PR https://github.com/appsmithorg/appsmith/pull/2996 so you're aware!

riodeuno commented 3 years ago

Hi @zegerhoogeboom, Thanks! I'll review this ASAP.

appsmithorg / appsmith