Skip to content

Conversation

@SeppPenner
Copy link
Contributor

@Joelius300
Copy link
Contributor

Thank you, it looks good but I have one concern.

I just realized from this PR that ArgValidation is only used in the callback classes and only one method is not about callback stuff. These callback classes are all deleted in #70 and replaced entirely. Also why is ArgValidation public?! I actually think we don't need ArgValidation anymore 😅

I won't merge this PR because it would just cause unnecessary merge conflicts when #70 is done but thank you, you did exactly what I meant by the card. This is a good reminder for me to do something about ArgValidation in #70.

@Joelius300 Joelius300 closed this Mar 29, 2020
@SeppPenner
Copy link
Contributor Author

I guess, you really need to finish #70 first before something else can be done 😄

No, it's ok. There needs a lot of stuff to be done in this project and since I have more time now due to the Corona stuff and home office now, I can again work on this project.

@Joelius300
Copy link
Contributor

Awesome, I'm happy you're here again :) Check what I wrote in the other closed PR just now, I appreciate the help.

Joelius300 added a commit to Joelius300/ChartJs.Blazor that referenced this pull request Mar 29, 2020
This class had mostly helper methods for the now replaced click and hover handlers. The null or whitespace checks are inlined everywhere so this class isn't used anymore. This was also mentioned in this [PR-comment](mariusmuntean#94 (comment)). If we have a helper class for argument validation, we need to actually use it.
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