mariusmuntean / ChartJs.Blazor

Brings Chart.js charts to Blazor
https://www.iheartblazor.com/
MIT License
676 stars 151 forks source link

Time scale line chart example #165

Closed mmuffins closed 3 years ago

mmuffins commented 3 years ago

I recently worked on a project where ChartJs.Blazor came in pretty hand, and since there was no example for time scale charts which I used in the project, I figured I could create one for #122.

The sample is pretty much just an extension of the existing basic line chart. I had to adapt the dataset generation a little to make it work with timeaxes and to highlight some of the specifics of this chart type, but overall there's nothing unexpected since I tried to stay in-line with both the existing samples and the ones from chartjs.

Let me know if you need any changes (or just adjust the PR).

Joelius300 commented 3 years ago

Hey @mmuffins, thanks for the PR!

I like the structure of your code, good job!
Some nit-picks I'll probably do when merging can be overlooked ;)

I didn't look at the result yet (I'll do when I get time) but one thing I noticed already, is that the moment.js was already there. Embarassingly, I put it after the Chart.js reference and since we weren't using time-charts yet, it didn't cause any issues but of course it needs to be loaded before Chart.js (see #162, whoops). Please remove that second reference to moment.js and let's forget about that 😅

Also, you are using random dates instead of incremental ones which IMO gives a weird feel. Do you think that would be something you could change? In the Chart.js samples they also use incremental dates (they often use incremental values on the x-axis).

mmuffins commented 3 years ago

Sure, no problem. I updated the sample to be less... uh, funky. I also changed the timescale to be dynamic instead of forcing always days to be displayed.

mmuffins commented 3 years ago

On a semi-related note, I also found a bug that affects most samples where an exception is thrown if the Remove Data button is clicked often enough.

I already fixed it in the current example but will go through the other samples and submit a new PR when I have some time. (I also opened #167 for it)

mmuffins commented 3 years ago

Alright, the errors when removing data should be fixed with the latest update. I actually wanted to create a separate PR for this to keep the issues clean, but kind of accidently checked in the changes to the wrong branch and the PR was automatically updated with them. I hope that's not an issue.

Joelius300 commented 3 years ago

First of all, thanks for the PR.

As you can see, I closed this in a commit. I decided to use your code as a basis and did some changes that I thought were appropriate and pushed it. It should be pretty close to the original now.

I mentioned you as co-author which means you should show up on the contributions graph and also get credit for adding/fixing that.

This also applies to #167.

For future reference, you should split the PR into their respective parts so the maintainers have an easier time reviewing and merging your contributions. For this specific case that would mean creating one PR for fixing #167 and another one for adding the time scale sample. If possible, you'd base both of these PRs on the current master and make them not depend on each other (again makes it easier to review, test and merge).

Thank you for your contribution!