testing timeSlider offset #1076
Conversation
There was a problem hiding this comment.
Code looks good (one suggestion).
I tested it by dragging a selection. Taking a screenshot. Refreshing the page. Taking another screenshot, and then comparing the screenshots.
Before this PR: the selection expands slightly.
After this PR: the selection is rock-solid.
:-)
| // and 'species collection start date' variable, | ||
| // such a 2 pixel results in about 4 months | ||
| const brushOffset = | ||
| xBrushScale.invert(2).getTime() - xBrushScale.domain()[0].getTime(); |
There was a problem hiding this comment.
I prefer this for readability and also just in case the domain doesn't start at zero one day!
I've tested it and it works fine.
| xBrushScale.invert(2).getTime() - xBrushScale.domain()[0].getTime(); | |
| xBrushScale.invert(2).getTime() - xBrushScale.invert(0).getTime(); |
There was a problem hiding this comment.
Oh, and we should add a comment referring to the SAFE_PIXEL constant in the visx source code
https://github.com/airbnb/visx/blob/86a851cb3bf622b013b186f02f955bcd6548a87f/packages/visx-brush/src/Brush.tsx#L14
(Shame they don't export it...)
And remove the console.logs before merging. Thanks!
There was a problem hiding this comment.
@bobular Thank you for your tests and reviews 👍 Sounds great it works! I followed your comments and made a new commit. Also, changed this PR to a regular one.
|
I'll merge it if you won't! |
|
Thanks @bobular I was just gauging whether it should be merged or not because of the release date 😅 Certainly you can merge it if you think it is okay :) |
|
Thanks for the consideration. The release already went out so all is good. Merging now. Thanks for tackling this proactively! |
This is a test to consider the inherent timeSlider's offset, 2 pixels.