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

[WIP] Interactive Legends #4317

Conversation

djbarnwal
Copy link
Member

To be merged after PR #4291

This PR attempts to solve Issue #1657. Legends can now respond to user clicks when legend: true is present within a selection containing a project transform.

arvind and others added 22 commits October 13, 2018 15:11
This commit helps verify that introducing the `init` property for selections should not affect their existing compile- or run-time functionality.
This commit helps verify that introducing the `init` property for selections should not affect their existing compile- or run-time functionality.
@arvind arvind self-assigned this Dec 4, 2018
@arvind arvind self-requested a review December 4, 2018 14:36
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

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

I think we will benefit from a broader syntax prototyping before diving into implementing one specific design. (We need to design the ideal syntax first.)

src/selection.ts Outdated
* A boolean determining whether the legend if present should be interactive
* or not
*/
legend?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Have you consider other alternative syntax for expressing this?

For example, if a selection is projected on X (encodings: ["x"]), what would legend: true do?

Also, if a selection is projected on Color (encodings: ["color"]), should legend be interactive by default?

const newValue = value ? value : {opacity: {value: 0.9}};
// To do : Add test case for legends and symbols
newValue.opacity = [
{test: `!(length(data(${store}))) || vlSelectionTest(${store}, {${field}: datum.value})`, ...newValue.opacity},
Copy link
Member

Choose a reason for hiding this comment

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

I recommend using the assembleSelectionPredicate method to generate this test string, so that we're not duplicating important logic. That method may require some minor modification to support the custom second argument to vlSelectionTest.

import {defaultTieBreaker, Explicit, makeImplicit, mergeValuesWithExplicit} from '../split';
import {UnitModel} from '../unit';
import {LegendComponent, LegendComponentIndex} from './component';
import * as encode from './encode';
import * as properties from './properties';

// For MVP, later remove global variable and create new types
export let selectionOnChannel: any = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

@arvind What would be the best way to send this information to a selection transform function. We can create this list using the model param but has method of a transform does not have a model argument to it. In theory, we can add that argument to the function. Moreover, we can also get this list when the model is being parsed by creating a new method at model.ts. But, both of these methods look like a long stretch to me. I think I am missing something of a very simple solution here. Any thoughts?

@arvind arvind force-pushed the as/refactorSelectionCodePaths branch 3 times, most recently from 8bf2208 to fe4b76c Compare March 8, 2019 15:00
@github-actions github-actions bot closed this Mar 8, 2019
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