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.

Sign up for GitHub

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

Jump to bottom

Bug when trying to use both 'href' and 'tooltip' #156

Merged
merged 17 commits into from
Jul 6, 2016
Merged

Bug when trying to use both 'href' and 'tooltip' #156

merged 17 commits into from
Jul 6, 2016

Conversation

faizan-khan-iit
Copy link
Collaborator

@faizan-khan-iit faizan-khan-iit commented Jun 29, 2016 •
edited
Loading

This PR covers Animint issues #100 and #155

faizan-khan-iit
faizan-khan-iit reviewed Jun 29, 2016
View reviewed changes
inst/htmljs/animint.js
@@ -1696,8 +1696,7 @@ var animint = function (to_select, json_file) {
return d["clickSelects.variable"] + " " + d["clickSelects.value"];
};
}
elements.text("")
.append("svg:title")
Copy link
Collaborator Author

@faizan-khan-iit faizan-khan-iit Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What is elements.text("") doing here? Removing it seems to solve the problem...

Edit: Got it. Need to think of some other approach. This won't do.

@coveralls
Copy link

coveralls commented Jun 29, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 806ada5 on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

faizan-khan-iit commented Jun 29, 2016 •
edited
Loading

After this minor change, everything seems to work fine.

Here is the example from #155 - Worldbank example
And here is the example from #100 - Visualization of Ridge Hike

But the wercker build is stuck for some reason. Maybe ignore it since the Travis build is passing?

Edit: Found a bug in my approach. Will probably need to revert that.

@tdhock
Copy link
Owner

tdhock commented Jun 30, 2016

Thanks for working on this Faizan. Before starting to code various solutions, can you please add at least one test case that fails?

@tdhock
Copy link
Owner

tdhock commented Jun 30, 2016

Also please add a test case for the bug in your approach that you found.

@coveralls
Copy link

coveralls commented Jun 30, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling f066c20 on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

faizan-khan-iit commented Jun 30, 2016 •
edited
Loading

@tdhock Sorry about that. I found out about the bug just before I had to leave so I thought to put in a comment so that you people do not waste your time.

So in the initial bug that we were trying to fix, <circle> was getting overwritten by elements.text("") when using tooltip with href since the both circle and title were children of anchor tag a. So I thought removing elements.text("") should do the job. Which led to this viz, as I mentioned above.

Initially it works fine, but while interacting with the points, the code keeps appending title elements one after the other, which is the bug I noticed. If you navigate to the above viz, initially all is good. Notice the Canada and US points in blue in the top left of the plot. But once you hide all the points excluding North America, you will see that their tooltip labels are now Armenia and Albania (that's what I get). The titles get updated every time I hide or unhide elements, and the topmost value is displayed every time.

Hence I currently remove <title> every time an update takes place. This results in this viz, which seems fine for all the testing I have put it through.

Hopefully this explains the changes and tests that I made.

Still the tooltip test gets stuck somewhere. I will see what is causing this.

EDIT: Running tests with Firefox seems to work well locally. The tests get stuck at the same point with PhantomJS.

I tried a lot of things including increasing Sleep time to 10 secs, but its not working. Even added a pause between each clickID call but still no good. It always gets stuck on the first getHTML call. Any ideas how to make this work with phantomjs?

@coveralls
Copy link

coveralls commented Jun 30, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 1eb14f0 on href+tooltip into f11dd20 on master.

@coveralls
Copy link

coveralls commented Jul 1, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 0c06500 on href+tooltip into f11dd20 on master.

@tdhock
Copy link
Owner

tdhock commented Jul 1, 2016

phantomjs has problems when there are several elements with the same ID -- is that the problem?

@faizan-khan-iit
Copy link
Collaborator Author

@tdhock I don't think so. The HTML is quite straight-forward and there are no ID clashes. I specify the IDs to be clicked here.

I tried to find out where the execution gets stuck, and I found out it gets stuck here, at the first getHTML() call.

Should I write a different test if this doesn't work?

@tdhock
Copy link
Owner

tdhock commented Jul 1, 2016

try removing non-alphanumeric characters from the ids (or convert them to underscores). I have a feeling that (&) characters are the problem.

@coveralls
Copy link

coveralls commented Jul 1, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 5e1611e on href+tooltip into f11dd20 on master.

@coveralls
Copy link

coveralls commented Jul 1, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 99a9393 on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

@tdhock Great observation! Once I filtered out the non-alphanumeric chars, the tests pass locally using tests_run(filter="tooltip").

But the bad news is that they get stuck when running tests_run(filter="renderer1"), like in the builds.

I separated the tests which are causing the problem. Probably ignore these for now? We could check this separately using firefox locally...

@tdhock
Copy link
Owner

tdhock commented Jul 1, 2016

Try renaming that test file to test-renderer4-tooltip.R and then adding TEST_SUITE=renderer4 to the travis and wercker builds.

@coveralls
Copy link

coveralls commented Jul 1, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling aeea6e1 on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

travis seems fine with the changes but looks like wercker is still stuck with the new tests.

@tdhock
Copy link
Owner

tdhock commented Jul 2, 2016

i restarted the wercker build so let's hope it magically fixes itself

otherwise i would recommend double-checking the id's rendered on that page

@faizan-khan-iit
Copy link
Collaborator Author

@tdhock Could you please restart again. It failed before starting the tests...

@tdhock
Copy link
Owner

tdhock commented Jul 4, 2016

looks like there are still problems. can you double-check everything on that page to make sure there are no duplicate id values, and no id values with non-alphanumeric characters? you may even want to try deleting _- characters to see if that helps, and make sure that the id does not start with a number. Not sure why all of this is necessary, I guess it is some bug in phantomjs...

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 061b1ed on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

faizan-khan-iit commented Jul 5, 2016 •
edited
Loading

@tdhock Looks like wecker build failed again without starting the tests..

I cut the region names to the first three letters. So these are the html id's we are working with now https://travis-ci.org/tdhock/animint/jobs/142332160#L2663

I will remove these once we complete the wercker build. Should I try and remove the rest of the underscores too? I would probably need to change the JS code, which may cause unexpected problems in the other tests.

Also there are no conflicts whatsoever in the rendered HTML. The only point worth noting is that the svg and td elements under these IDs mentioned have IDs with "_svg" and "_label" appended to them. Eg.
Legend ID: plot_ex_region_variable_eas
SVG ID: plot_ex_region_variable_eas_svg
Label ID: plot_ex_region_variable_eas_label

But that should not be problem since there are no clashes.

EDIT: We are now using Label IDs instead, but I don't know if that will work with wercker since the build failed before starting the tests again.

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 3b5bf70 on href+tooltip into f11dd20 on master.

@tdhock
Copy link
Owner

tdhock commented Jul 5, 2016

@cpsievert can you please give @faizan-khan-iit permissions to restart your wercker build?

faizan I don't understand why the wercker tests fail... can you try simplifying the test or using a different data set?

also while we debug this, feel free to start working on something else in another branch...

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 119f642 on href+tooltip into f11dd20 on master.

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 119f642 on href+tooltip into f11dd20 on master.

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling a29de98 on href+tooltip into f11dd20 on master.

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling 4a075e5 on href+tooltip into f11dd20 on master.

@faizan-khan-iit
Copy link
Collaborator Author

@cpsievert Thanks!

@tdhock Looks like the new tests work well with wercker. As for the older tests, I have not deleted those yet, but removed them from the test suites. If you approve of this, I could update the NEWS and DESCRIPTION.

A minor note: I had to install ggplot2 separately for wercker. Don't know why but it was not installing ggplot2 from our PR...

@coveralls
Copy link

coveralls commented Jul 6, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling dbee635 on href+tooltip into f11dd20 on master.

@tdhock
Copy link
Owner

tdhock commented Jul 6, 2016

+1 looks good to me, go ahead and increase the version in NEWS and DESCRIPTION, then you can merge with master

@coveralls
Copy link

coveralls commented Jul 6, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 1.147% when pulling bd97804 on href+tooltip into f11dd20 on master.

@faizan-khan-iit faizan-khan-iit merged commit 0373586 into master Jul 6, 2016
@faizan-khan-iit faizan-khan-iit deleted the href+tooltip branch July 6, 2016 16:35
This was referenced Jul 6, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@faizan-khan-iit @coveralls @tdhock