-
Notifications
You must be signed in to change notification settings - Fork 611
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 clearing selection #4720
Merged
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
7989028
added clear transform to files
958360c
added test suite and modified clear to not occur on default - must fl…
5821af8
adding single type support
ef5f6b8
adding test cases for single + interval clearing
dd06819
[Travis] Update examples (build: 22361)
de29ed0
Clear transform default doubleclick clears selected data and brush, w…
0c3d122
adding test cases for clear.ts and updating other test files to turn …
1505a1e
added clear transform to files
7249a10
added test suite and modified clear to not occur on default - must fl…
3cba482
adding single type support
d49d694
adding test cases for single + interval clearing
20eba3f
[Travis] Update examples (build: 22361)
cd6362a
Clear transform default doubleclick clears selected data and brush, w…
94027a9
adding test cases for clear.ts and updating other test files to turn …
f1f08d7
Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…
cb15c08
[Travis] Update schema (build: 22402)
2e8b2b5
[Travis] Update examples (build: 22402)
c9ab937
migrated clear to conditional config, and interval type clear now onl…
70b4ea3
Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…
e6335a1
adding clear transform documentation
d505802
Merge branch 'master' into ajl/clear
30857fe
[Travis] Update schema (build: 22802)
2ea1ead
[Travis] Update examples (build: 22802)
73777ca
simplified code a little
97ae4e3
Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…
ae4f8ae
clearing selection and panning/zooming, also updated docs and tests
3cdba25
[Travis] Update examples (build: 22820)
f0804b2
significantly refactored clear code
77aa146
Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…
d105409
updating TSDocs and Vega-lite docs
937aa9f
[Travis] Update examples (build: 22858)
1a3ec0c
Minor fixes to clear logic/style to minimize signal additions.
arvind 053f24b
Append clear to top-level signals to account for input bindings.
arvind 519a2b6
Refine documentation on selection clearing.
arvind 6b81dd2
Selection clearing should account for toggling.
arvind b99ef92
[Travis] Update schema (build: 22868)
f6e9a88
[Travis] Update examples (build: 22868)
9455fb8
Fix backticks
kanitw e51e6ce
Rename `selection_clear_heatmap` to `selection_heatmap` as it doesn't…
kanitw 165d8ca
Refactor selection_heatmap example + add it to the gallery
kanitw dd9d82e
[Travis] Update examples (build: 22876)
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default clear should be
mouseout
ifon
ismouseover
.(But
dblclick
is good otherwise. Thus, I don't thinkdblclick
should be defined here in config.) .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't think
"interval"
needs default "clear" given that click would be the default clear without an additional signal anyway. (Click triggers brush, and if you don't drag, that's equivalent to clearing.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is an interesting point. The default
"on": "click"
is defined in the configuration, so it seems to make sense to define clear consistently as well. But I see your point that if the user changed to"on": "mouseover"
we might want to detect that and reflect it. If that's the only exception we can think of, I'd rather handle it separately, when we parse defaults, rather than not haveclear
defined in ourSelectionConfig
whatsoever.I disagree. When an interval selection is bound to scales, it would be helpful to clear it to return to the original scale domains (so too if we had brush-to-zoom functionality as in #4742). Moreover, as much as possible, I would like to keep all selection types working consistently (that is, after all, our original vision of a grammar of interaction rather than a typology of selections).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the rest of the language, we make the config
undefined
if the default value has condition rule. For example, we don't setopacity
in mark config. Same for axislabelAngle
.When we want users to be able to customize config for each case, we then introduce a more specific version of config. For example,
bandPaddingInner
scale config isundefined
(automatic) by default, but the automatic rule will draw value frombarBandPaddingInner
andrectBandPaddingInner
scale configs. If users setbandPaddingInner
scale config to a specific value, then the default value is fixed andbarBandPaddingInner
andrectBandPaddingInner
are ignored.So I think we should have consistent config design here.
Good point. I agree.
Note that as I mentioned previously, I see the benefit of interaction typologies as a macro level on top of selection grammar and have started prototyping it a bit, but that's a different issue separate from this. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply, I just got back to the US.
Is the intention to have clear non-conditionally defined, i.e. set default configs to
dblclick
and in cases where user changed to"on: "mouseover"
they also need to change"clear": "mouseout"
. Or have it conditionally defined, making the configundefined
and conditionally choosing the EventStream trigger based off of theon
config?If the latter, should I make these changes in this PR or in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all.
I mean the latter:
Since it's a simple change, let's do in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, don't forget to add
clear
to the docs. :)