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

Firsttimers issue #10 #13

Merged
merged 5 commits into from Jul 6, 2018

Conversation

akashp-git
Copy link
Contributor

Pull request for the issue 'Add title to interactive content [mostly html changes, perhaps some js] (#10)'
Branch firsttimers-issue-#10 contains the changes done.

@yochannah
Copy link
Member

yochannah commented Feb 14, 2018

@amand1996 If you get time, if would be great if you could review this PR. Thanks!

@amand1996
Copy link
Member

amand1996 commented Feb 15, 2018

It looks good. Though I have some points in my mind. The code inserts an extra row to the table generated by imtables.js library for the purpose of table heading (while I think it would have been better not to manipulate the table generated by the library, rather one should insert a heading on top of the table div). Also, if the above is followed, I don't think there will be any need of setTimeOut.
(setTimeOut certainly does the work, but use of such looping mechanism is not a good practice)

@akashp-git
Copy link
Contributor Author

Hi @amand1996 thanks for reviewing the code. I'll try to make the changes and submit a new PR.

@akashp-git
Copy link
Contributor Author

Hi @amand1996 , there are two div elements with same id "left_div", the second div contains the table. So how should i proceed, should i change the id of any one of the div or access the element by its index after getting all the div elements by tag name ??

@yochannah
Copy link
Member

Renaming the div should be fine!

@akashp-git
Copy link
Contributor Author

PFA of the screenshot.

The first "left_div" contains the graph and the next "left_div" contains the table, please let me know which div to change and any relevant name for it.

capture

@yochannah
Copy link
Member

Use your judgement - we don't mind what they're named so long as they make sense! maybe "graph" and "table", or something like that?

Copy link
Member

@amand1996 amand1996 left a comment

Choose a reason for hiding this comment

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

Looks Good. :) 👍

@yochannah yochannah merged commit 0b55a78 into intermine:master Jul 6, 2018
@akashp-git
Copy link
Contributor Author

Hi @yochannah , @amand1996 thanks for merging the pull requests

@yochannah
Copy link
Member

@a99web sorry for it taking so long! I came to look on the repo for some other reason and thought "oh no, I'm so sorry I forgot you!" :) thanks for your help and patience!!

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

Successfully merging this pull request may close these issues.

None yet

3 participants