wadackel/sweet-scroll

Uncaught TypeError: Cannot read property 'stop' of undefined

nickclaw opened this issue · 14 comments

Looks like there might be a regression with v1.1.0.

The line that errors out is: https://github.com/tsuyoshiwada/sweet-scroll/blob/b0fee7600f3d0ae94d898e73eb366d736e8e6bcb/src/sweet-scroll.js#L264

The code that caused it is here:

  const input = document.querySelector(/* snip */);
  const container = document.querySelector(/* snip */);
  if (input && container) {
    const scroller = new SweetScroll({ duration: 200 }, container);
    scroller.toElement(input);
  }

Thanks for Issues 😃

I think the DOMContentLoaded has not been called.
It does not work in the following code?

// ...

if (input && container) {
  const scroller = new SweetScroll({
    duration: 200,
    initialized() {
      this.toElement(input);
    }
  }, container);
}

This code is called long after DOMContentLoaded is fired, it looks like this regression might be from your recent change to make the getContainer callback fire asynchronously (https://github.com/tsuyoshiwada/sweet-scroll/blob/28a3b90bd80e79b1d1f4c125c7bff0970f5ec0a7/src/sweet-scroll.js#L463).

I could be wrong but I think that makes a lot of the example on the README no longer valid

Using the initialized option does work btw, but I think this qualifies as a breaking change that might affect other people using the library like I am.

Oh, that makes sense...
I was just worried point to when want to update to v1.1.0.

I need your advice.
Currently, the inside get to container element waiting for DOMContentLoaded and load events.
By abolishing it entirely, can eliminate the asynchronous processing, such as initialized().
README will be in the following such description.

document.addEventLlistener("DOMContentLoaded", () => {
  const sweetScroll = new SweetScroll({ /* options */ });
  // ...
}, false);

Thank you for your valuable comments.

If probably went the above changes, it is likely to be released as v2.0.0.

I like that idea, I think it's pretty rare that people will be trying to get things to scroll before the DOM is actually loaded. It's generally a best practice for people to avoid running scripts until the DOM has actually been built anyways.

I think so, too. I heard that and was relieved 😃
Scheduled to v2.0.0 release that next week.
Thanks!

Hi @nickclaw.
Just it has been updated to v2.0.0.

Please check when you have time.
Thanks!

@tsuyoshiwada looks pretty good, unfortunately still getting the same error -- just for a different reason.

It looks like that in 2.0 if the container isn't found to be scrollable, this.tween is never set. In my case the container's children aren't always big enough to enable scrolling so it fails. If that's your intention that's fine, although in my opinion it'd be nicer to silently do nothing in that case.

Thanks you for check.
If possible would like to issue an error if can not find a scrollable element. (Because it will hinder debugging unless an error occurs.)

However, if there seems to be a need, I will consider a little.


If possible, I'd be happy if you give me the minimum sample code that can not find a container. (I think that it would be better if a fundamental solution could be made.)

@nickclaw Error handling was improved with v2.0.1.
Will keep silent unless explicitly specify the outputLog option.
Please check when you have time.

Sweet I'll check it out. Thanks a lot for all your help.

Your welcome. Thank you for giving me good advice