Consolidate charts

[mariusmuntean]

Removed the subclasses of ChartBase and renamed ChartBase to Chart.
The old way of creating a chart:

<ChartJsBarChart @ref="_barChart"	
                 Config="@_barChartConfig"	       
                 Width="600"
                 Height="300" />

becomes:

<Chart @ref="_barChart"
             Config="@_barChartConfig"
             TConfig="ChartJs.Blazor.ChartJS.BarChart.BarConfig"
             Width="600"
             Height="300"/>

This should make it easier to use charts since you don't need to know about each chart type and its Configuration type.
Note: for the moment you need to explicitly specify TConfig, but the Blazor team works on the possibility to infer that from the Config instance so in the future you can leave it out.

[Joelius300]

Good job on consolidating the components.
I see you also upgraded the sample projects nugets which is nice but not really in the scope of this PR.

However, this PR does way too many other things. There's no need to refactor/reformat code outside of the samples and the chart components. Also it edits tons of files by just removing unnecessary usings; this really shouldn't be done here. I don't think it makes sense to merge it like this.

The rest is highlighted by comments.

[Joelius300]

Looks good to me.

If it was me, I would nitpick those few indentation-inconsistencies that razor markup regularly messed up (for example the starting braces of collection-initializers is a razor favorite) and put some additional effort into the xml-comments on Chart.

Only two questions but they can both be addressed later so I'll approve this already.

• Would you consider removing the .Charts namespace and have Chart be top-level so ChartJs.Blazor.Chart? Plays into Use of ChartJs.Blazor outside Razor/Blazor #78.
• Do we need the _ViewImports.razor file in the Charts directory? As far as I'm concerned (tested it quickly), this is not needed.

[mariusmuntean]

I fixed a couple of indentation issues and removed the _ViewImports file.

Regarding the namespace suggestion: There are multiple issues with the file structure right now. For example ChartJs.Blazor.ChartJS, I bet we can come up with something better; The ExpandoObjectExtensions.cs should be moved to the Interop namespace; The Interop namespace could be better structured (e.g. pull out all the public Handlers to another namespace and let everything in Interop be internal).

After I've reorganized that it will become clearer where the new Char.razor component fits best.

[Joelius300]

Great! It would be nice to have the indentation fix in the "update samples" commit, consider doing that before merging.
Another note, I saw that you merged #112 with an ordinary merge-commit but I thought we agreed that rebase-merge should be preferred when possible right?

Regarding your comments about the namespaces, I have a few suggestions.

• First of all, read through Use of ChartJs.Blazor outside Razor/Blazor #78, I've already put a lot of ideas with background there
• Only ChartJsInterop is internal, IMethodHandler, JavaScriptHandler, DelegateHandler, etc are all public. Either we put all of those in top-level or we keep the namespace for both but I don't think it makes sense to have the Interop namespace for only one internal class.
• ExpandoObjectExtensions is only used by the interop but has no other connection to it. It doesn't make sense to move it to the Interop namespace.

[mariusmuntean]

Regarding the namespaces, I disagree. The Interop namespace should contain nothing public as it is of no concern to the library users. The ExpandoObjectExtensions contains only functionality for the interop layer, so they belong together.
The various Handlers shouldn't be just tossed in the topmost level namespace.

#78 is very desirable, but we've got a long way to go until we can tackle it. It is premature to guide our current decisions on that

[Joelius300]

I think our disagreement stems from the fact that the interop consists of two parts. One part is the interop to activate and update the chart and I 100% agree with you there, that shouldn't be exposed to the user (that's why ChartJsInterop is internal). However, there's also the second part of the interop and that's the callback stuff. That absolutely needs to be exposed to the user and is definitely also part of the interop. The delegates for the callbacks are stored somewhere in Common.Callbacks, we could move the interop stuff related to callbacks (IMethodHandler, DelegateHandler, etc) there; that would make sense to me.

Regarding ExpandoObjectExtensions I don't understand why you would want to move it to the interop stuff. Not a single line in that class is related to interop stuff. You need to think about encapsulation, just because it's only used in one place currently doesn't mean it belongs there.

[mariusmuntean]

I think we mostly agree. The callbacks should be moved together into a namespace that has nothing to do with interop.
The ExpandoObjectExtensions was specifically written for the interop code and that functionality is not used anywhere else. No other better reason is necessary for grouping them into the same namespace.

in my opinion this discussion about the namespace is totally misplaced here and I suggest we don't continue it.

[Joelius300]

I still strongly disagree on moving ExpandoObjectExtensions but we can talk about that later 👍