Adding CSV string import function #28
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jywarren
reviewed
May 21, 2019
| }); | ||
| }); | ||
| } | ||
| function colorBackground(ctx,canv){ |
There was a problem hiding this comment.
Hi! This is starting to mix in view code with parsing code, and it seems like maybe we need to put all views code in its own file, to follow an MVC model. What do you think?
|
This is looking really good. However, I want to encourage you to take a breather now both @namangupta01 and @IshaGupta18 and consider doing some code reorganizing to put views code in a different place from data processing code and that in a different place from data import code. Ideally, if you standardize the data format, you should be able to pass a standard data from any import source, to the processor, which does stuff, then passes a standard data format to the renderer (the view) for display. Does this make sense? It's easier said than done! But at 400+ lines, your code is now complex enough that it could use this kind of organization, and you'll benefit later in the project! Thanks!!! |
|
Yes, I absolutely agree with you. This code does need some refactoring and
reorganizing. So basically, to do this, I should separate thr script for
data parsing and views right? Basically, pass the final data produced after
the parsing functions to the view. Please correct me if I am wrong and
also, what should be a good practice in this case. Thanks a lot!
…On Tue, May 21, 2019, 9:41 PM Jeffrey Warren ***@***.***> wrote:
This is looking really good. However, I want to encourage you to take a
breather now both @namangupta01 <https://github.com/namangupta01> and
@IshaGupta18 <https://github.com/IshaGupta18> and consider doing some
code reorganizing to put views code in a different place from data
processing code and that in a different place from data import code.
Ideally, if you standardize the data format, you should be able to pass a
standard data from any import source, to the processor, which does stuff,
then passes a standard data format to the renderer (the view) for display.
Does this make sense? It's easier said than done! But at 400+ lines, your
code is now complex enough that it could use this kind of organization, and
you'll benefit later in the project!
Thanks!!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28?email_source=notifications&email_token=AJXHQZ4WFE5MYIHFBCQKF73PWQNKTA5CNFSM4HOKOA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4NCAY#issuecomment-494457091>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZZNBECSWRARLETDA7LPWQNKTANCNFSM4HOKOA7A>
.
|
|
As we also have to support mulitple graphs from multiple csvs. I was
thinking of using classes. And divide the work between various instance
methods. What do you think?
…On Tue, May 21, 2019 at 9:41 PM Jeffrey Warren ***@***.***> wrote:
This is looking really good. However, I want to encourage you to take a
breather now both @namangupta01 <https://github.com/namangupta01> and
@IshaGupta18 <https://github.com/IshaGupta18> and consider doing some
code reorganizing to put views code in a different place from data
processing code and that in a different place from data import code.
Ideally, if you standardize the data format, you should be able to pass a
standard data from any import source, to the processor, which does stuff,
then passes a standard data format to the renderer (the view) for display.
Does this make sense? It's easier said than done! But at 400+ lines, your
code is now complex enough that it could use this kind of organization, and
you'll benefit later in the project!
Thanks!!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28?email_source=notifications&email_token=AE6AEYMT33E6MNJF3PKN5QTPWQNKRA5CNFSM4HOKOA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4NCAY#issuecomment-494457091>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE6AEYOELSCPTMQIWF6XFSDPWQNKRANCNFSM4HOKOA7A>
.
|
|
Can we try to consult with @sagarpreet-chadha and @rexagod a bit on this? I
think it's a great moment to connect with others looking to organize their
code!
On Tue, May 21, 2019 at 1:24 PM Naman Gupta <[email protected]>
wrote:
… As we also have to support mulitple graphs from multiple csvs. I was
thinking of using classes. And divide the work between various instance
methods. What do you think?
On Tue, May 21, 2019 at 9:41 PM Jeffrey Warren ***@***.***>
wrote:
> This is looking really good. However, I want to encourage you to take a
> breather now both @namangupta01 <https://github.com/namangupta01> and
> @IshaGupta18 <https://github.com/IshaGupta18> and consider doing some
> code reorganizing to put views code in a different place from data
> processing code and that in a different place from data import code.
> Ideally, if you standardize the data format, you should be able to pass a
> standard data from any import source, to the processor, which does stuff,
> then passes a standard data format to the renderer (the view) for
display.
> Does this make sense? It's easier said than done! But at 400+ lines, your
> code is now complex enough that it could use this kind of organization,
and
> you'll benefit later in the project!
>
> Thanks!!!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#28?email_source=notifications&email_token=AE6AEYMT33E6MNJF3PKN5QTPWQNKRA5CNFSM4HOKOA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4NCAY#issuecomment-494457091
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AE6AEYOELSCPTMQIWF6XFSDPWQNKRANCNFSM4HOKOA7A
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28?email_source=notifications&email_token=AAAF6J4XFVCJJEDFRAM6AQTPWQV3HA5CNFSM4HOKOA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4TKYQ#issuecomment-494482786>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J7USYLRM2CJPJPEBBTPWQV3HANCNFSM4HOKOA7A>
.
|
|
MVC model is undoubtedly easy to grasp and understand . I think the distribution of files in all our projects should follow MVC guidelines . Now if you will see PublicLab.editor project , the classes (and class inheritance) are beautifully implemented in each module of the editor . I think use of classes there made the code very organized , but if you would see carefully the UI code in mixed with data-processing code in each module , right ? However as we have made various independent modules following same rules/functions to display UI , the code is easy to understand . Other approach is implemented in LBLD , as @jywarren mentioned earlier we have distributed different UI code in a different folder and Thank You 😄 ! |
|
That's some pretty great advice! I will look into the projects deeper and
see how it can be applied here. Thanks a lot!
…On Wed, May 22, 2019, 12:45 PM Sagarpreet Chadha ***@***.***> wrote:
MVC model is undoubtedly easy to grasp and understand . I think the
distribution of files in all our projects should follow MVC guidelines .
Now if you will see PublicLab.editor
<https://github.com/publiclab/PublicLab.Editor> project , the classes
(and class inheritance) are beautifully implemented in each module of the
editor . I think use of classes there made the code very organized , but if
you would see carefully the UI code in mixed with data-processing code in
each module , right ? However as we have made various independent modules
following same rules/functions to display UI , the code is easy to
understand .
Other approach is implemented in LBLD
<https://github.com/publiclab/leaflet-blurred-location-display> , as
@jywarren <https://github.com/jywarren> mentioned earlier we have
distributed different UI code in a different folder and
the data-processing code in the main folder . I think if a project is in
its initial stage --- we should follow this approach . Later when we have
the basic functionalities implemented , then it will be quite easy to move
code around to achieve better architecture .
UI files :
https://github.com/publiclab/leaflet-blurred-location-display/tree/main/src/ui
data-processing file :
https://github.com/publiclab/leaflet-blurred-location-display/blob/main/src/blurredLocationDisplay.js
Thank You 😄 !
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28?email_source=notifications&email_token=AJXHQZ45RVWUA2HWAF2BP43PWTXI3A5CNFSM4HOKOA7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV6EFZA#issuecomment-494682852>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJXHQZ5TNQAMAFYD5ZP4AVLPWTXI3ANCNFSM4HOKOA7A>
.
|
|
Okay so I am reading up about MVC structure in js, it is a little tough to understand, but I will try and figure it out. I was also looking at the LBLD structure, which is very similar to this code, and only the UI based code needs to be separated out. What do you think should be done for now? MVC would definitely be a better choice, but it might take a little longer to implement. @jywarren @namangupta01 |
|
Okay so after a lot of debugging and trial and errors, I have fixed the hovering error of the add graph feature (#24) and this is the latest PR having the latest UI design and all the features till now. I suggest merging this PR so that now, the MVC work can be done better. It is really important to merge this PR because all the latest code is contained in it. @jywarren @namangupta01 @gauravano kindly respond soon. Thanks a lot! |
|
OK, I'm good with this if you all are -- thanks a lot! Maybe with a review from @namangupta01 we can then go ahead and merge this? |
jywarren
approved these changes
May 25, 2019
|
This seems good to me. |
This was referenced May 28, 2019
Closed
IshaGupta18
added a commit
that referenced
this pull request
Aug 10, 2019
* Project Setup (#5) * PublicLab Grapher Project Setup * Added Jquery, Papaparse and chart.js dependencies in package.json (#7) * Adding the parsing feature according to new project structure * Added npm run script to watch changes and added browserify package for build (#10) * Added Mocha testing framework and chai for assertion and added some sample test (#13) * Plotting Graphs using Chart.js (#18) * Sample data generation and table display * emptying tables * Plotting graph usingchart.js * checking file type through drag and drop and displaying alert on invalid type * removing inline function calling * little refactoring * showing checkboxes against valid columns only * Adding graphing menu for different graph types * adding colors to graph * plotting the whole data * Add files via upload * Adding CSV string import function (#28) * Adding CSV string import function * small fix * keeping up to date * Added Multiple graphs Feature (#29) * Basic Implementation of Class * Add implementation of different table generation * Added Multiple Menu For multiple graphs * Multiple Chart Generation Completed * Minor Changes * Divided into Diiferent files * Added Babel for transpiling es6 code and modularize code by movig classes to seperate files * Minor Bug Removed * Fixed Minor linting issues * Added Patch for Multiple Graphs (#36) Patch For Multiple Graph Bug * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update README.md * Update README.md * Create CONTRIBUTING.md * Create CODE_OF_CONDUCT.md (#46) * Create PULL_REQUEST_TEMPLATE.md * Update issue templates * Adding ESlint (#37) * Create Downloadable Spreadsheet using SheetJS (#43) * Changes to src * More changes * remote url access * trying to pass remote values * SheetJS Complete * Resolved Bugs * All fixes! * Removing and adding some CDNs (font awesome addition and Range slider deletion) (#52) * Remove console.log's from View.js * changed the style of the button in View.js (#54) * [IMP] Added Tests, Refactored Code, Added Plotly.js and resolved some logical bugs (#59) * Shifting parsing code from View.js ro CsvParser.js * refactoring: adding return statements * Testing with refactored code * 1st test basic * resolving constrcutor error * Changing e=import and export syntax * try * Added Tests and corrected some code * ChartJs class * Plotly Class Added * Documentation for View and Csvparser functions (#61) * Added Installation Instruction (#62) * Update README.md * Gsheet (#63) * sign-in Google * attempt for google sheets * dummy credentials * ready to host on heroku * dummy * Update package.json * Update package.json * Update package.json * Bump lodash from 4.17.11 to 4.17.15 (#67) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.15. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.11...4.17.15) Signed-off-by: dependabot[bot] <[email protected]> * Update package.json * Update package-lock.json * Implementing CODAP export (#66) * updating package.json * heroku path * . * g sheet cred * sheet Functions * v1.0.4 * v1.0.5 * prev file use * view ma * latest V * Add file Description option to save against file * ids for popover fields * v1.2.1 * assigned title and desc * CODAP export * CODAP DONE * del creds * UI testing * v 1.3.2 * removing unused files * Delete test.csv * Delete uitest.js * UI Tests (#75) * updating package.json * heroku path * . * final tests1 * view changes
IshaGupta18
added a commit
that referenced
this pull request
Aug 20, 2019
* Project Setup (#5) * PublicLab Grapher Project Setup * Added Jquery, Papaparse and chart.js dependencies in package.json (#7) * Adding the parsing feature according to new project structure * Added npm run script to watch changes and added browserify package for build (#10) * Added Mocha testing framework and chai for assertion and added some sample test (#13) * Plotting Graphs using Chart.js (#18) * Sample data generation and table display * emptying tables * Plotting graph usingchart.js * checking file type through drag and drop and displaying alert on invalid type * removing inline function calling * little refactoring * showing checkboxes against valid columns only * Adding graphing menu for different graph types * adding colors to graph * plotting the whole data * Add files via upload * Adding CSV string import function (#28) * Adding CSV string import function * small fix * keeping up to date * Added Multiple graphs Feature (#29) * Basic Implementation of Class * Add implementation of different table generation * Added Multiple Menu For multiple graphs * Multiple Chart Generation Completed * Minor Changes * Divided into Diiferent files * Added Babel for transpiling es6 code and modularize code by movig classes to seperate files * Minor Bug Removed * Fixed Minor linting issues * Added Patch for Multiple Graphs (#36) Patch For Multiple Graph Bug * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update issue templates * Update README.md * Update README.md * Create CONTRIBUTING.md * Create CODE_OF_CONDUCT.md (#46) * Create PULL_REQUEST_TEMPLATE.md * Update issue templates * Adding ESlint (#37) * Create Downloadable Spreadsheet using SheetJS (#43) * Changes to src * More changes * remote url access * trying to pass remote values * SheetJS Complete * Resolved Bugs * All fixes! * Removing and adding some CDNs (font awesome addition and Range slider deletion) (#52) * Remove console.log's from View.js * changed the style of the button in View.js (#54) * [IMP] Added Tests, Refactored Code, Added Plotly.js and resolved some logical bugs (#59) * Shifting parsing code from View.js ro CsvParser.js * refactoring: adding return statements * Testing with refactored code * 1st test basic * resolving constrcutor error * Changing e=import and export syntax * try * Added Tests and corrected some code * ChartJs class * Plotly Class Added * Documentation for View and Csvparser functions (#61) * Added Installation Instruction (#62) * Update README.md * Gsheet (#63) * sign-in Google * attempt for google sheets * dummy credentials * ready to host on heroku * dummy * Update package.json * Update package.json * Update package.json * Bump lodash from 4.17.11 to 4.17.15 (#67) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.15. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.11...4.17.15) Signed-off-by: dependabot[bot] <[email protected]> * Update package.json * Update package-lock.json * Implementing CODAP export (#66) * updating package.json * heroku path * . * g sheet cred * sheet Functions * v1.0.4 * v1.0.5 * prev file use * view ma * latest V * Add file Description option to save against file * ids for popover fields * v1.2.1 * assigned title and desc * CODAP export * CODAP DONE * del creds * UI testing * v 1.3.2 * removing unused files * Delete test.csv * Delete uitest.js * UI Tests (#75) * updating package.json * heroku path * . * final tests1 * view changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@jywarren @namangupta01, in response to the comment #26 (comment), here's importing a CSV as a string, please give your reviews. Mentors, please give your inputs too. Thank you!