-
Notifications
You must be signed in to change notification settings - Fork 627
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
Initialize Selections #4139
Initialize Selections #4139
Conversation
c5c5c44
to
c475bb3
Compare
Tagged as blocked until #4068 is merged (which itself is blocked by vega/vega-parser#89). But, once the upstream PRs are merged, this one should be good to go. Compile- and runtime tests are passing locally. |
With this spec, an infinite loop occurs when initializing the interval selection. I suspect something to do with scale triggers on the filter. Marked this PR as WIP until I have a chance to resolve this bug. {
"layer": [
{
"mark": {"type": "point", "color": "lightgray"},
"selection": {
"box": {
"type": "interval",
"init": {"x": [50, 100], "y": [5, 10]
}
}
}
},
{
"mark": "point",
"encoding": {
"color": {
"type": "quantitative", "field": "Cylinders",
"legend": false
}
},
"transform": [{"filter": {"selection": "box"}}]
}
],
"encoding": {
"x": {"type": "quantitative", "field": "Horsepower"},
"y": {"type": "quantitative", "field": "Acceleration"}
},
"data": {"url": "data/cars.json"},
"$schema": "https://vega.github.io/schema/vega-lite/v2.4.1.json"
} |
After investigating this issue, I suspect there may be a bug in Vega (vega/vega#1428) where signals continue to listen for scale updates even if they're marked as |
130a791
to
a0f4bc9
Compare
a0f4bc9
to
018fea0
Compare
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.
Please update the documentation as well.
I'll make a full review after #4068 has been merged.
9a54954
to
d787382
Compare
@domoritz: As there'll be several documentation updates related to all the new selections work, I wanted to do all of it in a separate more comphreneisve pass/PR rather than piecemeal if that's ok. |
That makes sense. If you think this will take more than a week, please create an issue that describes what needs to be documented. |
Does this PR support initialization of selections with dates? What about booleans? I think it would be great if we supported the same types that are supported as values in encodings. |
@@ -4,7 +4,8 @@ | |||
"data": {"url": "data/cars.json"}, | |||
"selection": { | |||
"brush": { | |||
"type": "interval" | |||
"type": "interval", | |||
"init": {"x": [55, 160], "y": [13, 37]} |
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.
Shouldn't we see the brush in the SVG as well? See https://github.com/vega/vega-lite/pull/4139/files?short_path=b21ff41#diff-b21ff41954cf78b8b1141416d74085d5
Let me know when you get the chance to update the PR. I'd like to make a release soon so we can test these changes thoroughly. |
59101af
to
6914fe2
Compare
Supporting datetime objects proved trickier than I expected since it involves juggling expression strings with regular JSON primitives. But, I think I got it working. And also extended initialization to support initializing multi selections with an array of values. Unfortunately, merging this issue is blocked by three issues/PRs in Vega:
|
Okay, let's wait for a Vega release then. Luckily, no other PR is blocked by this and we can make a new Vega-Lite rc release without this PR. |
To clarify, the solution involving the |
329c30e
to
b968296
Compare
b968296
to
d110f89
Compare
763fce2
to
835e440
Compare
835e440
to
bd1b707
Compare
if (isIntervalSelection(selDef)) { | ||
selCmpt.init = parseInit(selDef.init); | ||
} else { | ||
const init = isArray(selDef.init) ? selDef.init : [selDef.init]; |
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.
We could use array again here once we have vega/vega#1618 in, but I'd say let's just merge 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.
That relies on the Vega 5 transition to let's do it later.
Need another review.
Not sure why the yarn lockfile needs to change but okay. |
Good catch! I think that's a result of auto-conflict merge. I'll remove it. |
🎉 |
const str = init.map(v => assembleInit(v, wrap)).join(', '); | ||
return `[${str}]`; | ||
} else if (isDateTime(init)) { | ||
return wrap(dateTimeExpr(init)); |
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.
Shouldn't this be dateTimeExpr(init, true)
?
This PR relies on the refactoring introduced by #4068, so please merge that one first.
This PR adds an
init
property to selection definitions to initialize them using the fields or encoding channels a selection is projected over. For example, here is an updated version of ourinteractive_query_widgets
example that initializes the selection to select cars with 8 cylinders manufactured in 1971.Similarly, an updated
interactive_brush
example to initialize the brush selection is as follows:Fixes #3409, #3431, and vega/altair#599.
/cc @jakevdp