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

fix: normalize selections across multi-views and selection emptiness #7204

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

arvind
Copy link
Member

@arvind arvind commented Feb 2, 2021

No description provided.

@arvind arvind requested review from a team and removed request for a team February 2, 2021 17:23
@arvind arvind marked this pull request as draft February 2, 2021 17:37
@arvind
Copy link
Member Author

arvind commented Feb 2, 2021

Hmm, looks like these changes are causing a lot more specs to be normalized even though there isn't any real change in the specification. Need to investigate why. @vega/vl-dev, would love any pointers to what I'm doing wrong. Nevermind, think I found the bug.

@arvind arvind marked this pull request as ready for review February 2, 2021 18:03
@arvind arvind requested a review from a team February 2, 2021 18:04
@arvind
Copy link
Member Author

arvind commented Feb 2, 2021

Ok, should be fixed.

src/normalize/selectioncompat.ts Outdated Show resolved Hide resolved
src/normalize/selectioncompat.ts Outdated Show resolved Hide resolved
src/normalize/selectioncompat.ts Outdated Show resolved Hide resolved
src/normalize/base.ts Show resolved Hide resolved
src/normalize/selectioncompat.ts Outdated Show resolved Hide resolved
}

return bin;
}

function normalizePredicate(op: any) {
function normalizePredicate(op: any, normParams: NormalizerParams) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this is old code but why is op any?

function normalizeTransforms(spec: any, normParams: NormalizerParams) {
const {transform: tx, ...rest} = spec;
if (tx) {
const transform = tx.map((t: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is t any?

bin: normalizeBinExtent(t.bin)
};
} else if (isLookup(t)) {
const {selection: param, ...from} = t.from as any;
Copy link
Member

Choose a reason for hiding this comment

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

Why is t.from any?

src/normalize/selectioncompat.ts Outdated Show resolved Hide resolved
}

public mapUnit(spec: UnitSpec<Field>) {
public mapUnit(spec: UnitSpec<Field>, normParams: NormalizerParams) {
const {selection, ...rest} = spec as any;
Copy link
Member

Choose a reason for hiding this comment

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

This is old code but why does spec need to be any?

@domoritz domoritz merged commit a72ea91 into as/selection-params Feb 5, 2021
@domoritz domoritz deleted the as/selectionCompatFixes branch February 5, 2021 17:59
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.

2 participants