mattosaurus / ChartJSCore

Implementation of Chart.js for use with .NET Core.
GNU General Public License v3.0
116 stars 34 forks source link

Implement public Options Options{ get; set; } #11

Closed ghost closed 6 years ago

ghost commented 6 years ago

Use the Options type for Chart options.

Using the Options type makes it a lot simple to use the Options property.

mattosaurus commented 6 years ago

Hi @perezLamed, I actually changed this from an Options type to an Object in a previous update as the chart specific options types such as BarOptions derive from and extend the Options type and trying to cast them back into an Options type loses the additional properties. Having Options as an object is certainly not the ideal solution but was the only way I could think of to get it to work correctly, have you got any better ideas? I was thinking of maybe initiating options like so but wasn't sure of the best way of going about it.

chart.Options = new Options<TOptions>

ghost commented 6 years ago

Thanks for the feedback @mattosaurus.

From my experience casting to a parent type does not loose properties. Can you give me a sample code of the problem and I will see if I can get it to work. I really do not like the "Object" type as it takes away all the advantages of c# and makes the code very messy.

mattosaurus commented 6 years ago

Hi @perezLamed, you're completely correct. Possibly this was an issue with an earlier version of .NET Core or the deserialization to JSON by Newtonsoft, either way having it as an "Options" type certainly seems to work now and (and is much tidier). I'll merge your changes and then push out an updated package. Thanks for your help.

Matt

ghost commented 6 years ago

Awesome!

ghost commented 6 years ago

Final note: I have also added a constructor that seems to make things work. (If the chart class do not create the Options then things do not work well).

    public Chart()
    {
        Options = new Options();
    }
mattosaurus commented 6 years ago

I didn't include an Options constructor because it's possible to create a chart without options and I figured the empty would be unnecessary in the resulting javascript. Or are you saying that this is causing other issues, besides if you try and set the options without creating a new object first?

ghost commented 6 years ago

@mattosaurus I agree with your logic. The main problem occur on the client side when adding new options to a current option object. Testing for (options == null) causes two path of execution. If it is already created - things are much simpler.

Example:

    public static void ChartOptions_SetTitle(Options options, string newTitle, int fontSize  = 20)
    {
        if (options == null) throw new ArgumentNullException(nameof(options), "Error! Options can not be null.");

        var title = new Title();
        title.Text = newTitle;
        title.FontSize = fontSize;
        title.Display = true;
        options.Title = title;
    }

Can be called like this ChartOptions_SetTitle(chart.Options, Title); No other option creations or tests are needed and it works for all cases. (I always prefer making things simpler & easier on the client side)