-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Line chart: fixed the check dataColors options array for emptyness #47
Conversation
@timqian Just wondering if you get chance to see this PR? |
@@ -127,7 130,7 @@ class Line { | |||
.attr('class', 'xkcd-chart-line') | |||
.attr('d', (d) => theLine(d.data)) | |||
.attr('fill', 'none') | |||
.attr('stroke', (d, i) => (this.options.dataColors | |||
.attr('stroke', (d, i) => (!isEmptyArray(this.options.dataColors) |
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.
if this.options
is undefined, looks the error still exists as we are we are still trying to get a property of undefined here
this.options.dataColors
.
May be check this.options && this.options.dataColors
here?
Not 100 present sure as I dont get a computer nearby to play with the code
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.
@timqian Thanks for getting back.
Correct, let me do that check and update this PR
@ajduke Thabks for the PR. And sorry for the late reply. I was doing some other stuff recently and haven't spent much time on this project recently. I will put more time on this repo after next week. About this PR, after a quick look at the change. I failed to tell if this is the best way or not. I will review it again soon |
Using |
https://github.com/timqian/chart.xkcd/pull/47/files#diff-fbc43a77b4e8fe64398aa79145272a6bR151-R152 Is that comment for the above code? instead of function for checking empty array, use |
Hi @ajduke By using |
Related issue
Fix #42
Due to
options
wasn't passed inLine
object, it was gettingoptions.dataColors
as the[ ]
empty due to which it wasn't getting any colors to plotScreenshot before and after this change
I was using
index.html
charts example to test things out, so i had used the emptyoptions
forLine
chart to replicatebefore
after