Skip to content
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

Integrating Simple-Data-Grapher with Plots2 #5993

Closed
wants to merge 23 commits into from

Conversation

IshaGupta18
Copy link
Collaborator

@IshaGupta18 IshaGupta18 commented Jul 8, 2019

This is the integration of simple-data-grapher with plots2

integ

integ2

  • The user can save file and use it later

  • The user can associate information with the file he/she uploads

  • A per-user data page to display different files uploaded by the user, where the user can download, delete the files and view the information (title and description) they associated with it.

@jywarren @namangupta01 @IgorWilbert @Souravirus @gauravano This is pretty big and important pull, please give your reviews and let me know what else can be done!

@IshaGupta18
Copy link
Collaborator Author

I would be needing some help here to resolve the errors which are bound to come in! Thanks a lot!

@CleverFool77
Copy link
Member

Hi @IshaGupta18 Isha, I wanted to ask that Regarding UI, Are you planning redesign or small fixes ?

@IshaGupta18
Copy link
Collaborator Author

@CleverFool77 I don't plan to completely redesign the simple-data-grapher page but would be improving its UI definitely. I am open to UI ideas for the list of files page though!

@CleverFool77
Copy link
Member

@CleverFool77 I don't plan to completely redesign the simple-data-grapher page but would be improving its UI definitely. I am open to UI ideas for the list of files page though!

Sure!!!!!! I'll try fetch this locally and try it and then tell you 🐙

@IgorWilbert
Copy link
Member

Hi @IshaGupta18 ! Your code looks great to me, except for some minor things like variable names. Do let me know if you need some help. Thanks!

@IshaGupta18
Copy link
Collaborator Author

Can someone help me here with the travis issues? @jywarren @gauravano ?

@Souravirus
Copy link
Member

Hi @IshaGupta18 what exactly is the problem with the travis?

@Souravirus
Copy link
Member

Also, It would be really nice if there is a button for the data files than going to that link through the browser. What do you think about this @IshaGupta18?

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jul 9, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jul 9, 2019 via email

@Souravirus
Copy link
Member

I think it would be better on the data-grapher itself for now. Later it can be placed on the profile page also.

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jul 9, 2019 via email

@IshaGupta18
Copy link
Collaborator Author

What about the travis and codeclimate issues?

@IshaGupta18
Copy link
Collaborator Author

Can we please try and fix them up soon so that the integration can be checked out? Thanks a lot everyone!

@CleverFool77
Copy link
Member

Hi @jywarren Does code climate doesn't check for linting ? Though it has been resolved now but earlier when there are were spacing and other stuff, it didn't seem to catch it. And this seems to be happening in other PRs too. Should we add a new linter ?

@IshaGupta18
Copy link
Collaborator Author

IshaGupta18 commented Jul 26, 2019 via email

@CleverFool77
Copy link
Member

CleverFool77 commented Jul 26, 2019 via email

Copy link
Member

@Anupam-dagar Anupam-dagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IshaGupta18 You missed spacing around + and some = operators. Please add them.
Also can you post the screenshot with the error header?

@IshaGupta18
Copy link
Collaborator Author

Here's the screenshot
image

@Anupam-dagar
Copy link
Member

@IshaGupta18 The screenshot with error header on the plots2 not the code.

@IshaGupta18
Copy link
Collaborator Author

Do you mean a permalink?

@Anupam-dagar
Copy link
Member

A screenshot of how the error will appear to user on the webpage.

@@ -18,30 +18,30 @@
<script>
var headerContainer = document.getElementsByClassName("body-container")[0];
SimpleDataGrapherObject = new SimpleDataGrapher("first");
var value='<%= current_user %>';
var value = '<%= current_user %>';
<% if current_user %>
SimpleDataGrapherObject.view.createButtons("yes");
var saveButton = SimpleDataGrapherObject.view.elementId+"_save_CSV";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed space around + operator here and the line below it.

@IshaGupta18
Copy link
Collaborator Author

How should I test that? Could you please tell?

I can tell you that it will occur like a red bootstrap alert:
image

@Anupam-dagar
Copy link
Member

To test the error block, you need to cause error first on purpose. This can be done by either changing ajax url to an incorrect url, or you can just turn off the server where the request is being sent. You can also provide incomplete data in the request. This will be a good test also to see if the error block is indeed getting executed on real errors.

@IshaGupta18
Copy link
Collaborator Author

image
Here you go!

let divAlert = document.createElement('div');
divAlert.classList.add("alert");
divAlert.classList.add("alert-success");
divAlert.innerHTML = "File save successfully!";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File saved successfully! will better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a typo. apologies!

@Anupam-dagar
Copy link
Member

There is an inconsitency in the + icon of the create spreadsheet button. I guess it needs to be fixed on the simple data grapher itself. Other than that, after fixing the typo of success alert, this PR LGTM. 🎉

@IshaGupta18
Copy link
Collaborator Author

Thank you! Finally yayy!

@plotsbot
Copy link
Collaborator

plotsbot commented Jul 27, 2019

2 Warnings
⚠️ New migrations added. Please update schema.rb.example by overwriting it with a copy of the up-to-date db/schema.rb. Also, be aware to preserve the MySQL-specific conditions for full-text indices.
⚠️ There was an error with Danger bot’s Junit parsing: No JUnit file was found at output.xml
3 Messages
📖 @IshaGupta18 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.
📖 #
Screenshots 📸 (click to expand)

5993-test_viewing_question_post.png

5993-test_wiki.png

5993-test_tag_page.png

5993-test_blog_page_with_location_modal.png

5993-test_login.png

5993-test_wiki_page_with_inline_grids.png

5993-test_questions.png

5993-test_methods.png

5993-test_tag_by_author_page.png

5993-test_viewing_the_dropdown_menu.png

5993-test_comments.png

5993-test_stats.png

5993-test_tags.png

5993-test_people.png

5993-test_front.png

5993-test_signup.png

5993-test_questions_shadow.png

5993-test_blog.png

5993-test_question_page.png

5993-test_front_page_with_navbar_search_autocomplete.png

5993-test_viewing_the_dashboard.png

Learn about automated screenshots

Generated by 🚫 Danger

@IshaGupta18
Copy link
Collaborator Author

@Anupam-dagar can you help me out with this error which has just occurred?

@IshaGupta18
Copy link
Collaborator Author

@cesswairimu please help me here with the travis error, I am not sure what problem occured as I made no serious change!

@IshaGupta18
Copy link
Collaborator Author

moved to #6113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants