search

timqian / chart.xkcd

xkcd styled chart lib
https://timqian.com/chart.xkcd/
MIT License
7.61k stars 200 forks source link

Line chart: fixed the check dataColors options array for emptyness #47

Closed ajduke closed 3 years ago

ajduke commented 4 years ago

Related issue

Fix #42

Due to options wasn't passed in Line object, it was getting options.dataColors as the [ ] empty due to which it wasn't getting any colors to plot

Screenshot before and after this change I was using index.html charts example to test things out, so i had used the empty options for Line chart to replicate

before

image

after

image

ajduke commented 4 years ago

@timqian Just wondering if you get chance to see this PR?

timqian commented 4 years ago

@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

timqian commented 4 years ago

Using object.assign to define default values seems a better way: https://github.com/nfriedly/express-rate-limit/blob/master/lib/express-rate-limit.js#L5

ajduke commented 4 years ago

Using object.assign to define default values seems a better way: https://github.com/nfriedly/express-rate-limit/blob/master/lib/express-rate-limit.js#L5

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 Object.assign. Is my understanding correct?

timqian commented 4 years ago

Hi @ajduke I fixed this issue on https://github.com/timqian/chart.xkcd/commit/cef9ba26d5e27fb75ae028c536dabd3a4d143489#diff-fbc43a77b4e8fe64398aa79145272a6bR33

By using Object Spread( the Object.assign in es6).