Joelius300 / ChartJSBlazor

This library is a modification of the awesome ChartJs.Blazor library by mariusmuntean. It's supposed to add more functionality to the LineChart and generally make the library more complete.
Other
40 stars 6 forks source link

Implemented all special enums. #87

Closed SeppPenner closed 5 years ago

SeppPenner commented 5 years ago

Fixes https://github.com/Joelius300/ChartJSBlazor/issues/9, Fixes https://github.com/Joelius300/ChartJSBlazor/issues/41.

SeppPenner commented 5 years ago

From https://github.com/Joelius300/ChartJSBlazor/pull/84#issuecomment-531697951:

SeppPenner commented 5 years ago

Fixes https://github.com/Joelius300/ChartJSBlazor/issues/85 as well.

Joelius300 commented 5 years ago

Still a few things missing:

Which tag? <code></code>? Or do you mean using <see cref="Enum.Center"/>?

Instead of writing When 'center' is set, blabla.. you should now write When <see cref="Enum.Center" /> is set, blabla... There are many places where this needs to be changed. These include (but there are probably many more):

This is also part of what I meant by The summaries of many properties need to be rewritten or adjusted.

Since string enums can be null, I don't think we should set a default value and just let chart.js pick the appropriate one. Example of this is in PointLabels.FontStyle. Only use a default value if the default isn't null. --> Please check this, the default under https://www.chartjs.org/docs/latest/axes/labelling.html is 'normal', not null.

You missed my point. When we specify a value, anything other than null, the property will be set in js. If we set a null value (or nothing which implicitly sets null for reference types) the corresponding js-property will be undefined. If a js-property is undefined in a chart.js construct, the default option will be used by chart.js. This means that if we manually set FontStyle.Normal, it will always use that. If we set it to null, chart.js will see undefined and will use the default which is 'normal'.
To summarize: it will use 'normal' either way BUT if we set null we let chart.js choose the default which is not only more correct, it also shifts the responsibility of choosing the right default for that property to chart.js which is generally what we want considering chart.js is huge.

Also something we should continue doing is marking these enums as sealed. All the new ones need this and two we missed as well. The ones we missed are SteppedLine and AxisDisplay.

SeppPenner commented 5 years ago

Should be all done.

Joelius300 commented 5 years ago

I don't understand what's going on with CubicInterpolationModes summaries. 'default' went to <see cref="CubicInterpolationMode.Monotone" /> somehow. Also I made a stupid mistake when I said put it in the class summary. I meant the property summary of monotone. That's the one that actually makes sense :)

Otherwise it looks good 👍

SeppPenner commented 5 years ago

Do you mean like this: https://github.com/Joelius300/ChartJSBlazor/pull/87/commits/46131334fbe0cfa579d3c32ac39936bb76d30515?

Joelius300 commented 5 years ago

Do you mean like this: 4613133?

Yes exactly :) I'll look over it in detail now and if I don't find any bigger issues, which I couldn't quickly resolve myself, I'll merge it soon (hopefully, I'm in class).

Joelius300 commented 5 years ago

I have finished working on this and could merge this now. However I cannot push my commit mqttnettest:special-enums :/ I get a 403.. do you maybe need to add me as a colaborator @SeppPenner ?

SeppPenner commented 5 years ago

Oh, sorry. Yes, you're added now.

SeppPenner commented 5 years ago

Nice 👍