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

Time scale line chart example #165

Closed
wants to merge 6 commits into from
Closed

Conversation

mmuffins
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

mmuffins commented Nov 26, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

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!

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