Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andreassundstrom
Copy link

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 include int?-valuesit as well.

Copy link
Contributor

@Joelius300 Joelius300 left a 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 :)

Comment on lines 26 to 33
{
"Title": "Stepped",
"Path": "charts/line/stepped"
}
{
"Title": "Stepped",
"Path": "charts/line/stepped"
},
{
"Title": "Interpolation",
"Path": "charts/line/interpolation"
}
Copy link
Contributor

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 :/

Copy link
Author

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.

@andreassundstrom
Copy link
Author

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 .editorconfig, if you don't want it I can remove it from the PR.

Best regards
Andreas

@Joelius300
Copy link
Contributor

Hey @andreassundstrom

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.
I can't tell you what will happen to your sample but feel free to leave this PR open, maybe someone will find it useful ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants