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

New feature legend decode added #160

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

viswaraavi
Copy link
Collaborator

When the user hovers on the element the respective legend text gets highlighted and gets bold with increase in font size. When hovers out the legend text becomes normal.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.3%) to 0.861% when pulling 829e2e5 on viswaraavi:master into 3a11efd on tdhock:master.

@viswaraavi
Copy link
Collaborator Author

I am still working on this to make the build pass. I think this change "return unsafe_name.replace(/[^-A-Za-z0-9_]/g,'');" is what making the tests to break otherwise I could not see any reasons why tests are breaking

@tdhock
Copy link
Owner

tdhock commented Aug 4, 2016

Hey @viswaraavi thanks for the PR. Do you have any examples of this feature working? If so, can you please run animint2gist and post a link to the resulting data viz?

After that, can you please think about writing a test for this new feature? Apparently we can simulate mouseover/out events in RSelenium ropensci/RSelenium#59

@viswaraavi
Copy link
Collaborator Author

This is the example for the new feature added
http://bl.ocks.org/viswaraavi/raw/2e323641b70f9ca904dd44e11b6bc0e9/

@tdhock
Copy link
Owner

tdhock commented Aug 4, 2016

wow very nice work!

in my opinion it would be less visually startling if the text size did not change. that has the effect of moving the entire legend. it would be better if the only thing that changes is the legend entry itself. can you please try just using bold text, without changing the text size?

also have you considered what to do for multiple legends? you could very well have a single data point which should highlight an entry in more than one legend. for example aes(color=sex, fill=smoker)

@viswaraavi
Copy link
Collaborator Author

Now i think the support for multiple legends work
http://bl.ocks.org/viswaraavi/raw/e85bba5637ebd43e237dc8a8a6e7ad2e/
But still i need to know in what other ways can the multiple legend will be created

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.3%) to 0.861% when pulling 87620e2 on viswaraavi:master into 3a11efd on tdhock:master.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.3%) to 0.861% when pulling 87620e2 on viswaraavi:master into 3a11efd on tdhock:master.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.3%) to 0.861% when pulling 3870ab1 on viswaraavi:master into 3a11efd on tdhock:master.

@viswaraavi
Copy link
Collaborator Author

1.I could not understand why animint2gist tests are failing.
2.In case of world bank dataset the legend does not highlight for population, this is because the data which is used to draw the geom_point in d3 does not have showselectedlegendsize as attribute instead has size as attribute. Can i know the reason for this?. If this is the case then somehow I should map between the size of the data and legend or else i have to change the compiler code to make this work

@tdhock
Copy link
Owner

tdhock commented Aug 5, 2016

looks good for the example with multiple legends. thanks for not changing the text size.

however the entire legend is still moving. one way to fix that would be to increase the height of the legend SVGs from 14 to 16 https://github.com/tdhock/animint/blob/master/inst/htmljs/animint.js#L1876

animint2gist tests always fail on PRs from other people, so that is fine, don't worry about it

about the size legend in the worldbank example - it is a continuous legend, not a discrete legend. showSelected variables are automatically created only for discrete legends. for now (this PR) I think it is OK to ignore continuous legends.

for a future PR it would be nice to do something like http://www.highcharts.com/maps/demo/map-drilldown/dark-unica for continuous legends

@viswaraavi
Copy link
Collaborator Author

I will try to fix the problem with the height. And try to write the tests for mouse hover events

@tdhock
Copy link
Owner

tdhock commented Sep 12, 2016

any update @viswaraavi ?

@viswaraavi
Copy link
Collaborator Author

l will start working on it again. May be this weekend. I was having classes

@tdhock
Copy link
Owner

tdhock commented Nov 14, 2016

hey @viswaraavi I merged your other PR, so it looks like this one now has some conflicts that need to be resolved.

By the way, I added you as a collaborator, so now you have the power to add whatever you like to animint. However, please do not push changes to master, unless absolutely necessary. For future PRs (not this one), please use branches of tdhock/animint (then the animint2gist test will not fail).

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.

3 participants