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

docs: Add example with hover path and search box #3459

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

dsmedia
Copy link
Contributor

@dsmedia dsmedia commented Jul 3, 2024

  • point paths on hover
  • search box to filter by country name

Relates to #2980

TODO:

  • Investigate and potentially correct North Korea and South Korea data in vega_datasets' gapminder json, which appears to have swapped labels.

Thanks for contributing to Altair!

Please follow these guidelines when submitting your PR:

  • Describe the purpose of the changes in PR, so that it is easy to understand the implication of the suggested changes.
  • Include unit tests and documentation for new features
  • Ensure that the title is a concise semantic commit message (e.g. "feat: Add embed_options to charts").
    • Append ! if the change is breaking (e.g. "fix!: Raise error when embed_options is None")

@dsmedia dsmedia changed the title Add example with hover path and search box docs: Add example with hover path and search box Jul 3, 2024
- point paths on hover
- search box to filter by country name

Relates to [vega#2980](vega#2980)

TODO:
- Investigate and potentially correct North Korea and South Korea data in vega_datasets' gapminder json, which appears to have swapped labels.
@mattijn
Copy link
Contributor

mattijn commented Jul 4, 2024

Thanks for this PR! Looks great, but a little bit big for my screen and adoption in the example page.

Can you add nearest=True within the following line?:

hover = alt.selection_point(on='mouseover', fields=['country'], empty=False)

Another thing, it is recommended to use the approach as is documented in https://github.com/vega/altair/blob/main/CONTRIBUTING.md#adding-examples, especially:

Once you have an example you would like to add there are a few guide lines to follow. Every example should:

  • have a arguments_syntax and methods_syntax implementation. Each implementation must be saved as a stand alone script in the tests/examples_arguments_syntax and tests/examples_methods_syntax directories.

Thanks again for this PR and hopefully you are able to improve this PR using these these suggestions.

@dsmedia
Copy link
Contributor Author

dsmedia commented Jul 5, 2024

The addition of nearest=True adds a nice visual effect of keeping one path visible at all times once the mouse enters the chart. This serves as a reminder that there is a time element that can be interacted with, not only a cross-section of countries. Without this, a user may not realize there is the option to see both panel and time-series data at the same time, unless they happen to mouse over directly on a mark.

One concern about adding nearest=True is that it somewhat alters the experience of mousing over a country and then trying to scroll through the year slider while keeping in focus that country's path over time, as the movement of the mouse toward the slider tends to trigger another country's path to become highlighted as it passes near to another country.

Any thoughts on this? One thought is to keep nearest as False but then to have one country's path shown in the chart's initial state by default. I'm not sure how to implement that, though.

@mattijn
Copy link
Contributor

mattijn commented Jul 5, 2024

Good point. Maybe like this?

hover = alt.selection_point(on='click', fields=['country'], empty=False, value='Egypt')

Its not really a hover anymore like this though..

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

This is great, thank you @dsmedia ! I made a few inline comments with suggestions. Regarding nearest I think the current behavior where the country line is not highlighted when the slider is moved is totally fine. I see that as a drill down action to see the entire trajectory of a single (or a few) countries, whereas the slider is more for seeing how all countries evolve over time as a group.

- Improve visual design and interactivity
- Add point opacity on hover for better focus
- Implement dynamic background year display
- Adjust chart dimensions and padding
- Refine region mapping and color scheme
- Update axis scaling and formatting
- Enhance country label positioning and styling
- Optimize code structure and comments
- Note: arguments syntax version will be added after further iteration of the example
@dsmedia
Copy link
Contributor Author

dsmedia commented Jul 6, 2024

Thanks for all the feedback @mattijn and @joelostblom. I've incorporated as best I could, while also fixing an issue with hover lines connecting out of sequential order, switched mark_line to mark_trail (which I think gives an intuitive sense of the sequential order) and made some other stylistic modifications, such as setting the country label on hover to appear above the point with the largest y-value for the country. I wasn't sure if there was a consensus about adding nearest=True but chose to leave it out as it seemed to make the hovering experience a bit too bouncy. But I'm happy to add it back in if that's the consensus. I will add the other syntax once the iteration on this vesion is complete.

@joelostblom could you check the behavior when the enter key is hit on the search box? I'm not sure what's expected but it seems to clear the visualization when I tested it in an interactive window in vscode. Also on the search box, do there happen to be any configuration options (autocomplete?) for the search box that are worth demonstrating here, since I think it'll be its first appearance in an example?

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

Great idea with the trail to indicate direction! I only have a few minor comments, so feel free to address those as you see fit and add the methods syntax version of the example also and then I think we can merge.

Regarding the behavior of the search box when enter is hit, I see the same as you both in VS Code and JupyterLab. I'm not sure how to change it, but we don't need to worry about it here, should probably be fixed on the Vega side of things.

Regarding autocomplete, Vega would need to add support for the datalist parameter and a way to pass a datalist to search input. I had opened an issue for it in the past here vega/vega#3702, but didn't get any insights

- Revise example description to clarify applicability to panel time series data
- Update x-axis label to make the units more understandable
- Streamline throughout and reduce unnecessary padding
- Exclude North Korea and South Korea due to data discrepancy in vega-datasets source
@dsmedia
Copy link
Contributor Author

dsmedia commented Jul 7, 2024

I see above "Some checks were not successful" with "lint / ruff-mypy (pull_request)" failing. Is there something I should do to fix that?

@dangotbanned
Copy link
Member

I see above "Some checks were not successful" with "lint / ruff-mypy (pull_request)" failing. Is there something I should do to fix that?

Will be fixed once #3463 is merged

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

This is looking great! Thanks for all your work in putting this example together and unveiling the data error as well @dsmedia ! Tests should pass now so I will go ahead and merge when they finish running!

@joelostblom joelostblom merged commit eeecb66 into vega:main Jul 8, 2024
11 checks passed
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.

4 participants