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

Adjust timeline zoom sensitivity on Firefox #2777

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jul 31, 2017

Resolves #2768 by taking the defaultWheelDelta logic from https://github.com/d3/d3-zoom.

Zooming transitions were made also animated to make them smoother (especially on Firefox which sometimes does discrete jumps).

Edit: Tracking of zoom events was wrapped into a debounce method to prevent excessive Mixpanel reporting.

@fbarl fbarl self-assigned this Jul 31, 2017
@fbarl fbarl requested a review from davkal July 31, 2017 13:35
@fbarl fbarl force-pushed the adjust-timeline-firefox-zoom-sensitivity branch 2 times, most recently from 006f81f to 83e433e Compare July 31, 2017 15:39
Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

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

Really difficult to test this on FF. Both master and this branch have a baseline of 75% CPU use. The adjustment seems to help with zoom sensitivity but is overshadowed by general lag. Is it just my machine?

The zoom track still seems a bit excessive. What do you hope to learn? If it's binary use vs non-use, maybe use some once logic, or otherwise massively turn up the debounce interval (request handling eats into drawing performance).

FWIW Chrome works perfectly fine.

I recommend changing the tracking, then merge this, and then do profiling for FF in a separate PR.

@rade
Copy link
Member

rade commented Aug 1, 2017

The FF performance is probably #2302.

@fbarl
Copy link
Contributor Author

fbarl commented Aug 1, 2017

The zoom track still seems a bit excessive. What do you hope to learn? If it's binary use vs non-use, maybe use some once logic, or otherwise massively turn up the debounce interval (request handling eats into drawing performance).

I hope to see at which depth the users are using the timeline, i.e. whether they are more interested in moving through minutes/hours/days/etc... With that in mind, I will increase ZOOM_TRACK_DEBOUNCE_INTERVAL from 1s to 10s and also replace the durationPerPixelInMilliseconds float with a rounded humanized time unit (minutes, hours, etc..) to make it easier to interpret the Mixpanel data.

@fbarl fbarl force-pushed the adjust-timeline-firefox-zoom-sensitivity branch from 83e433e to 5692429 Compare August 1, 2017 14:24
@fbarl
Copy link
Contributor Author

fbarl commented Aug 1, 2017

@davkal I've updated the PR.

@rade Maybe you'd want to test the FF behaviour (modulo lagging) before I merge it.

@rade
Copy link
Member

rade commented Aug 1, 2017

Maybe you'd want to test the FF behaviour (modulo lagging) before I merge it.

Nah, it can hardly have gotten worse ;)

@fbarl fbarl merged commit 9ea6626 into master Aug 1, 2017
@fbarl fbarl deleted the adjust-timeline-firefox-zoom-sensitivity branch August 8, 2017 14: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.

3 participants