-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Line Chart Interpolation sample #174
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
You ported the master-version of the sample which works but isn't optimal since we are still using Chart.js 2.9. Could you adjust the config to the according sample (note the 2.9 branch instead of the master branch)?
Apart from that there are some whitespace issues and I have left additional comments for further improvements :)
{ | ||
"Title": "Stepped", | ||
"Path": "charts/line/stepped" | ||
} | ||
{ | ||
"Title": "Stepped", | ||
"Path": "charts/line/stepped" | ||
}, | ||
{ | ||
"Title": "Interpolation", | ||
"Path": "charts/line/interpolation" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should stay two-space-indented. You might need to edit this file outside of Visual Studio, it can mess with the json indents :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is in .editorconfig
, it has default to indent_size = 4. Is it ok to add:
[samples.json]
indent_size = 2
I will regardless of course edit the file to have correct spacing.
ChartJs.Blazor.Samples/Client/Pages/Charts/Line/Interpolation.razor
Outdated
Show resolved
Hide resolved
ChartJs.Blazor.Samples/Client/Pages/Charts/Line/Interpolation.razor
Outdated
Show resolved
Hide resolved
ChartJs.Blazor.Samples/Client/Pages/Charts/Line/Interpolation.razor
Outdated
Show resolved
Hide resolved
ChartJs.Blazor.Samples/Client/Pages/Charts/Line/Interpolation.razor
Outdated
Show resolved
Hide resolved
ChartJs.Blazor.Samples/Client/Pages/Charts/Line/Interpolation.razor
Outdated
Show resolved
Hide resolved
…azor according to PR comments.
Thanks for feedback @Joelius300. Sorry for the misses in indentation, that was sloppy of me. I've tried to correct the PR according to your comments. I left a question about the Best regards |
It looks pretty good and I'm really sorry to just leave you hanging here but I'm leaving this community. Thank you for your understanding. |
Issue #122
Original sample
Original sourcecode
I want to contribute to the samples. I've added a sample for the interpolation-modes in ChartJS. I'm fairly new to the github-community (not C# or dotnet-core though) so I apologize if I should have made anything different in submitting this PR. Please let me know if you want me to improve anything in the code.
I made a copy of the
Pages/Charts/Line/Basic.razor
-file and modified it. The original sample had NaN-values in it so i modified the line to includeint?
-valuesit as well.