airbnb / visx

🐯 visx | visualization components
https://airbnb.io/visx
MIT License
19.5k stars 716 forks source link

initialBrushPosition and the first call to update the brush are slightly out of sync #1342

Open JoeVanGundy opened 3 years ago

JoeVanGundy commented 3 years ago

Update: have since discovered this is actually a bug in Visx (or unexpected behavior)

initialBrushPosition and the brush seem to be slightly out of sync. The first time the brush is used after a initialBrushPosition has been set,

Reproduction steps:

  1. Set the brush with initialBrushPosition
  2. Observe the values that are being set (Mon Aug 15 2011 00:00:00 GMT-0700 (Pacific Daylight Time))
  3. Move the left side of the brush to an earlier date.
  4. Notice how the right side of the brush is updated when it's not suppose to be: (Tue Aug 16 2011 06:11:10 GMT-0700 (Pacific Daylight Time))

Causing lots of issues for us since we're using the brush for day selection.

Is this a known issue or expected?

Here are steps to reproduce the bug in the base brush example: https://codesandbox.io/s/hopeful-cache-xu025?file=/Example.tsx

alexandrbig commented 2 years ago

Hello @hshoff @williaster ,

actually the values are not slightly out of sync, they are out of scale and domain.

In the demo open the console and click the brush selection. You'll see that zero value is not 0 it shows -2.000000000542981, more over the last position is higher than xMax value. See the screenshot.

Could you explain this behavior and how to fix it?

Thank you!

image

alexandrbig commented 2 years ago

@JoeVanGundy I managed to fix it by using new xScale for brush only:

const xScaleBrush = useMemo(
    () =>
      scaleTime({
        domain: xScale.domain(),
        // https://github.com/airbnb/visx/blob/master/packages/visx-brush/src/Brush.tsx#L119
        // visx brush adds 2 pixels for some reason to the origin range
        range: [-2, xMax + 2],
      }),
    [xScale, xMax],
  );

Anyhow this addition looks strange, there is no explanation why and for what. I can only assume that it is because of the stroke width of the brush rect. But i might be wrong.

alexandrbig commented 2 years ago

Unfortunately the fix provided above doesn't fix the whole issue with this "additions" as we have the range 4px wider for the brush in comparison to a normal scale.

alexandrbig commented 2 years ago

Currently fixed this issue in a following way:

const brushSafeDiff = useMemo(() => {
    // Brush adds 2 pixels to the xScale range, we have to calculate this diff beforehand to extract/add it to bounds
    return xScale.invert(2).getTime() - xScale.domain()[0].getTime();
  }, [xScale])

and then in onChange method:

onChange = (bounds) => {
      if (bounds) {
        const xBounds = [
          Math.max(bounds.x0 + brushSafeDiff, xScale.domain()[0].getTime()),
          Math.min(bounds.x1 - brushSafeDiff, xScale.domain()[1].getTime())
        ] as [number, number];
...

So we extract and add these 2 px each time when we get brush bounds.