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

For mark overlays, do not apply selection to the augmented layers yet. #3703

Merged
merged 1 commit into from May 6, 2018

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented May 4, 2018

(We will properly do that later in #3702)

@kanitw kanitw requested a review from domoritz May 4, 2018 22:50
@kanitw kanitw changed the title [Fix] For mark overlays, do not apply selection to the augmented layers yet. For mark overlays, do not apply selection to the augmented layers yet. May 6, 2018
@domoritz
Copy link
Member

domoritz commented May 6, 2018

What problem does this fix?

@kanitw
Copy link
Member Author

kanitw commented May 6, 2018

We don't have a clear policy how to do selection for overlay / composite yet #3702 . This resets the behavior to be "doing nothing". The old code was weird anyway.

@domoritz
Copy link
Member

domoritz commented May 6, 2018

Why was the old code weird? I don't think it's good to break selections for overlaid plots. Can you provide some examples where the old code was incorrect or what won't work anymore when we merge this?

@kanitw
Copy link
Member Author

kanitw commented May 6, 2018

The old code remove selection from the area layer and add selection to both point and line overlays. Isn't that weird?

@kanitw
Copy link
Member Author

kanitw commented May 6, 2018

This PR makes the logic for overlay supposes to just add new overlay layers, but preseves selection to be in the original layer. I think this is a reasonable standard logic. We could have more sophisticated policy later.

@kanitw kanitw merged commit 5a27289 into master May 6, 2018
@kanitw kanitw deleted the kw/overlay-selection branch May 6, 2018 19:58
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.

None yet

2 participants