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

Add support for multi-data tooltips #382

Merged
merged 7 commits into from
Jul 29, 2023

Conversation

noahrhodes
Copy link
Contributor

@noahrhodes noahrhodes commented Apr 8, 2021

Setting tooltips for multiple components was not working (see #332)

augument_encoding_type did not run correctly for an array that was passed in as the tooltip information.

this change adds augment_encoding_type(x::Array, data::Vega.DataValuesNode) for array types. It takes arrays and runs augment_encoding_type on the internal components

This code snippet now works:

using VegaLite, VegaDatasets

iris = dataset("iris")|>
@vlplot(
    tooltip=[{field="species"},{field="sepalLength"}],
    data=iris,
    :point,
    x=:petalWidth,
    y=:petalLength,
    color=:species,
)

There is a line in the docs/src/examples/examples_table_based_plots.md that can be updated if this change is added #tooltip=[{field="Origin",type="nominal"},{field="Cylinders",type="quantitative"}] #array not supported

This is my first contribution to the package. Is there anything else I should add to this pull request?
This fixes the problem and passes the tests, but I am unsure if there are other implications. This may not even be the correct way to fix this problem, and I am happy to take pointers to a different approach.

Take arrays and run augment_encoding_type on the internal components
Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

Awesome, that does look correct to me!

Could you maybe add a test that exercises this new code path?

I think eventually we would probably also want a simple =[:foo, :bar] to work, but we can do that in a follow-up PR.

src/vlspec.jl Show resolved Hide resolved
src/vlspec.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2021

Codecov Report

Merging #382 (5e6ad7c) into master (28c9ad9) will increase coverage by 2.01%.
Report is 50 commits behind head on master.
The diff coverage is 77.27%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
+ Coverage   76.90%   78.92%   +2.01%     
==========================================
  Files          12       12              
  Lines         433      427       -6     
==========================================
+ Hits          333      337       +4     
+ Misses        100       90      -10     
Flag Coverage Δ
unittests 78.92% <77.27%> (+2.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/VegaLite.jl 100.00% <ø> (ø)
src/vlspec.jl 77.63% <40.00%> (-2.66%) ⬇️
src/rendering/show.jl 76.92% <88.23%> (+11.92%) ⬆️

... and 3 files with indirect coverage changes

@noahrhodes
Copy link
Contributor Author

Changes made:

  • Removed println statements
  • Changed ::Array to ::AbstractArray
  • Added a test for the code path
  • Updated docs example to use array instead of a calculation for hover text

As for the test, I wasn't sure what to really test, or where to put it. I added a test that ran a @vlplot macro with tooltip array, and checked that the tooltip encoding is an array.

I think eventually we would probably also want a simple =[:foo, :bar] to work, but we can do that in a follow-up PR.

It works just fine as is!

Let me know if there are any further changes to be made!

@noahrhodes
Copy link
Contributor Author

Would it be possible to review and merge this sometime in the near future? It obviously hasn't been urgent, but a new issue was created about (#415) multiple field tool-tips.

@rtedwards
Copy link

Bump on this PR. Would love to see this merged. @noahrhodes @davidanthoff

@davidanthoff davidanthoff merged commit 1922f10 into queryverse:master Jul 29, 2023
12 checks passed
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.

4 participants