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

onCropComplete is firing on window resize #309

Open
gregg-cbs opened this issue Nov 6, 2021 · 6 comments
Open

onCropComplete is firing on window resize #309

gregg-cbs opened this issue Nov 6, 2021 · 6 comments
Labels
good first issue Good for newcomers PR-welcome help is welcomed for this issue

Comments

@gregg-cbs
Copy link

Describe the bug
If you resize the window, onCropComplete fires.

To Reproduce
Steps to reproduce the behavior:

  1. Go to this codesandbox
  2. Put a console log in the the onCropComplete
  3. Resize the window
  4. Notice console logs with what seems like with no debounce

Expected behavior
Not expecting onCropComplete to fire on window resize.. i understand why this might be in place for responsive reasons. But if it is a required feature I would expect the window resize to be debounced.

@ValentinH
Copy link
Owner

Having this callback fired is mandatory as the crop data is dependent on the window size. However, you have a good point regarding deboucing. We should add a debounce here.

Would you like to do a PR?

@gregg-cbs
Copy link
Author

Sure I can do that. Do you mind if I use the debounce package?

@ValentinH
Copy link
Owner

No it's fine as it's small enough 🙂

@gregg-cbs
Copy link
Author

Great will find some time in the next day or so and comment the PR here.

@ValentinH ValentinH added good first issue Good for newcomers PR-welcome help is welcomed for this issue labels Nov 15, 2021
@gregg-cbs
Copy link
Author

Looks like window resize uses the computeSizes method

  componentDidMount() {
    window.addEventListener('resize', this.computeSizes)
 }

  computeSizes = () => {
    const mediaRef = this.imageRef || this.videoRef
    ...
  }

If i debounce the resize I will have to pass the same debounced function into removeEventListener.

Would it be safe or okay to debounce the computeSizes method so that recalculations are never running wild:

  computeSizes = debounce(() => {
    const mediaRef = this.imageRef || this.videoRef
    ...
  }, 200)

or wrap it just for the resize

  debouncedComputeSizes = debounce(this.computeSizes, 200)

  componentDidMount() {
    window.addEventListener('resize', this.debouncedComputeSizes)
  }

  componentWillUnmount() {
    window.removeEventListener('resize', this.debouncedComputeSizes)
 }

@ValentinH
Copy link
Owner

Indeed, it's better to only do the debounce for the resize event. computeSizes is used in multiple places and in some it has to be done immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers PR-welcome help is welcomed for this issue
Projects
None yet
Development

No branches or pull requests

2 participants