FormidableLabs / victory

A collection of composable React components for building interactive data visualizations
http://commerce.nearform.com/open-source/victory/
Other
11.02k stars 524 forks source link

Weird zoom in/out! #2761

Closed omid closed 2 months ago

omid commented 9 months ago

Describe the bug You can zoom out out of the chart, and the zoom out is weird then!! I don't know how to describe it. watch the screencast below!

Victory version Your version at: https://formidable.com/open-source/victory/docs/victory-zoom-container/

Code Sandbox link Your online example: https://formidable.com/open-source/victory/docs/victory-zoom-container/

To Reproduce First, zoom in, and then zoom out many times, out of the chart. Like the screencast here.

https://github.com/FormidableLabs/victory/assets/45714/8dde276e-0004-4f24-a5d9-928120ab0150

Desktop (please complete the following information):

carbonrobot commented 9 months ago

The zoom is relative to your cursor position, try zooming out / in with your cursor centered in the graph

omid commented 9 months ago

@carbonrobot first of all, why can I zoom off the chart? second, it's not about that, watch the screencast more carefully, when I am off the chart, I ONLY zoom out, but the chart is in a loop of zoom-in/out!

carbonrobot commented 9 months ago

Your cursor appears to be still over the chart. Remember, the chart has padding all the way around and fills the entire column.

watch the screencast more carefully

Obviously, it is unclear what your mouse is doing, because we can't see what buttons are being pressed via the video.

I am unable to replicate that behaviour using Edge/Chrome/Firefox/Safari.

omid commented 9 months ago

Sure you cannot see what I do with my mouse buttons :D But I'm telling you what I'm doing.

Steps to reproduce:

  1. Go inside and zoom in
  2. Go in the white part, out of the chart and zoom out a lot
carbonrobot commented 9 months ago

Everything appears to be working as intended. The zoom is correct relative to the mouse position in the data points.

Could you describe the behavior you are expecting?

zoom

omid commented 9 months ago

what you have done is correct, but zoom out more than that, don't stop zooming out. to face the issue earlier, you don't need to zoom in a lot, one step is enough.

carbonrobot commented 9 months ago

I'm still unclear what you are asking. Are you talking about the numbers repeating due to the datum using Math.Pi and Sin?

omid commented 9 months ago

It's generally about both axises, but in the video you can see x-axis. In the video below, I ONLY ONCE, scroll up (zoom-in) and after that JUST scroll down (zoom-out). The scroll down is happening FIRST out of the chart THEN inside and outside of the chart.

https://github.com/FormidableLabs/victory/assets/45714/09976235-6c22-4859-b619-4b1f35803ba9

carbonrobot commented 9 months ago

The video looks normal to me, I'm still unclear on the question.

Here is the charts bounding box (it receives mouse events inside this red border)

image

Could you describe the problem with the following format?

  1. Test steps
  2. Expected result
  3. Actual result
omid commented 9 months ago

I used this script to show my mouse events :D window.addEventListener("wheel", event => console.log(event.wheelDelta > 0 ? "scroll-up (zoom-in)" : "scroll-down (zoom-out)"));

Steps:

In the video below, there is ONLY ONE zoom in, and after that, all others are zoom out!

https://github.com/FormidableLabs/victory/assets/45714/8cf9c3f1-1dff-4ef1-b87c-391543bf6a84

carbonrobot commented 9 months ago

Mouse wheel events have a deltaX, deltaY, and deltaZ property. I think the code you are looking for is

window.addEventListener("wheel", event => console.log(event.deltaY > 0 ? "scroll-up (zoom-in)" : "scroll-down (zoom-out)")); 
omid commented 9 months ago

Don't debug my code snippet, debug the library :D In both cases, the bug is there and is the same.

carbonrobot commented 9 months ago

Do you see the problem in this screen cap? I'm not seeing anything incorrect, but maybe we are talking about different things.

zoom2

omid commented 9 months ago

yes, I see that :)

when you did 26 scroll-ups, it means ZOOM OUT... but actually the chart zooming in.

To see the bug better, never do scroll-down after the first one. do scroll-down JUST ONCE

carbonrobot commented 9 months ago

I'm not seeing a bug, I'm just seeing the designed behavior. Can you describe the actual VS expected behavior?

carbonrobot commented 9 months ago

Perhaps there is some confusion in this example because it uses an initial zoom domain (in other words, the charts starts pre-zoomed-in).

You can paste the following code into the "Editable Source" box to see if that changes anything for you.

class App extends React.Component {
  constructor(props) {
    super(props);
  }
  state = {
    data: this.getScatterData()
  }
  getScatterData() {
    return range(50).map((index) => {
      return {
        x: random(1, 50),
        y: random(10, 90),
        size: random(8) + 3
      };
    });
  }
  render() {
    return (
      <VictoryChart
        containerComponent={<VictoryZoomContainer/>}
      >
        <VictoryScatter
          data={this.state.data}
          style={{
            data: {
              opacity: ({ datum }) =>  datum.y % 5 === 0 ? 1 : 0.7,
              fill: ({ datum }) => datum.y % 5 === 0 ? "tomato" : "black"
            }
          }}
        />
      </VictoryChart>
    );
  }
}
ReactDOM.render(<App/>, mountNode)

I think we are more than happy to change something, we just need to know what you would like changed.

omid commented 9 months ago

same :/

https://github.com/FormidableLabs/victory/assets/45714/97423742-2023-4b7f-afaa-5b241c1fa587

omid commented 9 months ago

@carbonrobot can we meet somewhere? google meet or discord or wherever you can?

github-actions[bot] commented 5 months ago

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

Aryan-mor commented 5 months ago

I have the same issue

omid commented 5 months ago

Here is an activity:

carbonrobot commented 5 months ago

As always we welcome and encourage contributions and PRs from the community. Please see our call to action for contributions and our code of conduct.

github-actions[bot] commented 2 months ago

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

omid commented 2 months ago

crcarlo commented 2 months ago

I think I understand the issue here.

Steps to reproduce

I found the following behavior on the first (sin) example here https://commerce.nearform.com/open-source/victory/docs/victory-zoom-container#victoryzoomcontainer

Tested on latest chrome with both MacBook trackpad and Apple Magic Mouse

Current behavior

The chart zooms out on the Y axis (until reaching the limit) while it zooms in on the X axis.

Expected behavior

It should zoom out on the X and Y axis until reaching the maximum zoom out limit for both axis just as if you were zooming out with the cursor inside the chart.

I can take a look at it and try to solve this.

simoneb commented 2 months ago

Great analysis and solutioning @crcarlo 👌