publiclab / simple-data-grapher

Turns CSVs into graphs in a few simple steps; embeds onto other websites!
https://publiclab.github.io/simple-data-grapher/
GNU General Public License v3.0
39 stars 52 forks source link

[IMP] Added Tests, Refactored Code, Added Plotly.js and resolved some logical bugs #59

Closed IshaGupta18 closed 5 years ago

IshaGupta18 commented 5 years ago

This is in continuation of #58 , but still, I'll copy the purpose of that PR:

  1. Shift some parsing logic from View.js to CsvParser.js, making View.js completely independent of parsing logic.
  2. Till now, the CsvParser.js file had functions that were interdependent on each other ie. one function calling the other, which created problems in debugging and testing. Now, all the functions are independently written, each returning the required value and assigning to the variables. A functionHandler function calls all the functions in the required order. This way, if we get a bug, we can pinpoint the exact function which is causing a problem, instead of tracing the entire thing through function calls.

These changes will definitely increase the readability of the code for us as well as new-comers.

Apart from the above, I have added some major tests for checking the parsing and data processing functions of CsvParser.js. This is a major thing as we have been going on about testing for a very long time, and it took me a lot of effort in understanding how it works, and it has been very helpful as it has improved the code quality.

I have also corrected some logical bugs in the code.

I have also created 2 different classes for Plotly.js library and Chart.js and an adapter function which can seamlessly switch between the 2 libraries, just by specifying the name of it. This has majorly reduced the code length of View.js and modularized the functionality.

This is an important PR which aims to achieve many important goals. @jywarren @namangupta01 @gauravano @Souravirus @rexagod @IgorWilbert kindly have a look at it, and I hope we can merge it soon enough, as a lot of work will depend on this one! Thanks a ton!

jywarren commented 5 years ago

This is SO COOL!!!! Great work @IshaGupta18 your architecture work is really paying off.

jywarren commented 5 years ago

I think this looks great. Do you think you're getting to the point where you can provide stable documentation to guide people on how to install and use this library, or are things still moving around and getting re-structured a bit too much?

Great work!

jywarren commented 5 years ago

Perhaps one thing you may start to think about is organizing your source files by where they fit into the big picture - like, input modules, output modules, UI/view versus parsing.

https://github.com/IshaGupta18/simple-data-grapher/tree/plotlyClass/src

Here are a couple examples of code organization along those lines:

https://github.com/publiclab/infragram/tree/main/src

https://github.com/publiclab/PublicLab.Editor/tree/master/src

IshaGupta18 commented 5 years ago

Yes I think we should document this more, I think @namangupta01 is writing the installation part of readme, but I think I might start writing a readme like the one you suggested in the gitter chatroom. And yes, maybe we can start making folders in the src folder, although we might need a little more clarity there. Thanks a ton!

On Tue, Jun 25, 2019, 12:37 AM Jeffrey Warren notifications@github.com wrote:

Perhaps one thing you may start to think about is organizing your source files by where they fit into the big picture - like, input modules, output modules, UI/view versus parsing.

https://github.com/IshaGupta18/simple-data-grapher/tree/plotlyClass/src

Here are a couple examples of code organization along those lines:

https://github.com/publiclab/infragram/tree/main/src

https://github.com/publiclab/PublicLab.Editor/tree/master/src

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/simple-data-grapher/pull/59?email_source=notifications&email_token=AJXHQZ5VYGWIPEXUZRN3FIDP4ELO5A5CNFSM4H274BCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYN5DBY#issuecomment-505139591, or mute the thread https://github.com/notifications/unsubscribe-auth/AJXHQZ2VQZE2SY5XPBQOCQLP4ELO5ANCNFSM4H274BCA .

IshaGupta18 commented 5 years ago

I am merging this one! @jywarren I will add the documentation very soon! Thanks a lot!

namangupta01 commented 5 years ago

Hi Isha, i have the pr ready, just have to make some lil changes. Will create pr today. Thanks!!!

On Wed, Jun 26, 2019 at 12:00 AM Isha Gupta notifications@github.com wrote:

I am merging this one! @jywarren https://github.com/jywarren I will add the documentation very soon! Thanks a lot!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/simple-data-grapher/pull/59?email_source=notifications&email_token=AE6AEYJNWRQ5WT52NVDXISTP4JP45A5CNFSM4H274BCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYRFNVA#issuecomment-505566932, or mute the thread https://github.com/notifications/unsubscribe-auth/AE6AEYP6DPJKLVF4RCZHG73P4JP45ANCNFSM4H274BCA .

IshaGupta18 commented 5 years ago

Okay cool, I'll see if anything else might be needed there. Thanks!