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

Allow selection.style getter in event handlers #2273

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Conversation

alexcjohnson
Copy link
Collaborator

Fixes #2244

@monfera you're right, d3.svg.brush doesn't work in a way that my idea of disabling the check inside it would work. But here's an alternative: if d3.event is truthy then we know we're in a user-initiated event, and so at least within the contexts of our own test systems we know the plot must be visible because the user is interacting with it!

Seem reasonable?

@monfera
Copy link
Contributor

monfera commented Jan 20, 2018

That's a fantastic idea 💃 ! Event handlers are often used to initiate userland changes to the scenegraph via style or attribute setting, eg. changing the color of something on hover, while d3.event is truthy. So, to clamp down on permissiveness, we could combine this with the cursor check. Probably D3 itself has very limited use of getters and we can add to the whitelist one by one if we encounter another, or we grep all D3 uses from the sources beforehand. But maybe I'm overthinking it, it's a great PR as it is!

@alexcjohnson
Copy link
Collaborator Author

Great, I'll merge it. As the whole initial render of any chart happens in our image tester and test dashboard with d3.event=null, I can't see much that we could be missing by allowing this exception as it is here, and I'd rather not accidentally break any other acceptable uses by trying to be overly cautious.

@alexcjohnson alexcjohnson merged commit d9cf4a5 into master Jan 21, 2018
@alexcjohnson alexcjohnson deleted the unstrict-events branch January 21, 2018 03:07
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