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 null to make the accompanying text in sync #4563

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@clemens-tolboom
Copy link
Contributor

clemens-tolboom commented Feb 19, 2019

The text on https://vega.github.io/vega-lite/docs/bind.html

With single selections, the bind property follows the form of Vega’s input element binding definition to establish a two-way binding between input elements and the selection.

fails when 'select all' is done by clicking on the graph to unselect ie Japan.

Please:

  • Make your pull request atomic, fixing one issue at a time unless there are multiple relevant issues that would not make sense in isolation. For the latter case, please make atomic commits so we can easily review each issue.
    • Please add new commit(s) when addressing comments, so we can see easily the new changeset (instead of the whole changeset).
  • Provide a test case & update the documentation under site/docs/
  • Make lint and test pass. (Run yarn test. If your change affects Vega outputs of some examples, re-run yarn build and run yarn build:example EXAMPLE_NAME to re-compile a specific example or yarn build:examples to re-compile all examples.)
  • Rebase onto the latest master branch.
  • Provide a concise title that we can copy to our release note.
    • Use imperative mood and present tense.
    • Mention relevant issues. (e.g., #1)
  • Look at the whole changeset as if you're a reviewer yourself. This will help us focus on catching issues that you might not notice yourself. (For a work-in-progress PR, you can add "[WIP]" prefix to the PR's title. When the PR is ready, you can then remove "[WIP]" and add a comment to notify us.)

Pro-Tip: https://medium.com/@greenberg/writing-pull-requests-your-coworkers-might-enjoy-reading-9d0307e93da3 is a nice article about writing a nice pull request.

Add null to make the accompanying text in sync
The text

> With single selections, the bind property follows the form of Vega’s input element binding definition to establish a two-way binding between input elements and the selection. 

fails when 'select all' is done by clicking on the graph to unselect origin.
@clemens-tolboom

This comment has been minimized.

Copy link
Contributor Author

clemens-tolboom commented Feb 19, 2019

Hmmm ... checking the vega way https://vega.github.io/vega/examples/interactive-legend.vg.json I see something like

{
      "name": "selected",
      "on": [
        {"trigger": "clear", "remove": true},
        {"trigger": "!shift", "remove": true},
        {"trigger": "!shift && clicked", "insert": "clicked"},
        {"trigger": "shift && clicked", "toggle": "clicked"}
      ]
    }

so I guess my PR is a workaround for a vega bug regarding the select bind:

  • not adding a -- all -- value
  • having a select multiple to support shift && click

to support this vega style behaviour.

@domoritz domoritz requested a review from arvind Feb 19, 2019

@domoritz

This comment has been minimized.

Copy link
Member

domoritz commented Mar 6, 2019

@arvind @jheer is this related to a Vega or Vega-Lite bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.