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

scrollIntoView Promise doesn't resolve on some elements #344

Closed
matthew-dean opened this issue Jul 12, 2018 · 11 comments · Fixed by scroll-into-view/smooth-scroll-into-view-if-needed#127

Comments

@matthew-dean
Copy link

I discovered an apparent bug, where I have something like this:

scrollIntoView([HTMLElement], {
  scrollMode: 'if-needed',
  block: 'nearest',
  boundary: document
}).then(() => { console.log('done') })

It appears that if the element doesn't need to be scrolled into view (or some other reason?), then the Promise will never resolve. Even if I change .then() to .finally(), nothing happens. No console.log is output. Meaning I can't chain scrollIntoViews.

I didn't have this problem until the element I needed to check if it needed to be scrolled into view was sometimes the same height as the document.

@stipsan
Copy link
Member

stipsan commented Jul 12, 2018

Hey there!

Are you talking about smooth-scroll-into-view-if-needed?

@matthew-dean
Copy link
Author

@stipsan Yes, that's what I'm using.

@stipsan
Copy link
Member

stipsan commented Jul 12, 2018

Alright. Then it's for sure a bug. It's always returning a Promise.all:
https://github.com/stipsan/smooth-scroll-into-view-if-needed/blob/c8aa5a2d23541c8f24921f0196ece43843c58f0d/src/index.ts#L111-L125

If scrollMode: 'if-needed' is breaking the behavior it sounds like it's returning something other than an empty array. Gonna check it out, thanks for the report! 😄

stipsan added a commit to scroll-into-view/smooth-scroll-into-view-if-needed that referenced this issue Jul 12, 2018
Fixes use cases where code like this might never resolve:
```js
scrollIntoView([HTMLElement], {
  scrollMode: 'if-needed',
  block: 'nearest'
}).then(() => { console.log('done') })
```

Fixes scroll-into-view/scroll-into-view-if-needed#344
@stipsan
Copy link
Member

stipsan commented Jul 12, 2018

I just released v1.1.13 with a fix, let me know if it works 😄

@stipsan
Copy link
Member

stipsan commented Jul 12, 2018

I'm open for feedback on what is returned when the promise resolved. It might make sense to include the same array of actions that it's mapping through?

@matthew-dean
Copy link
Author

Thanks for the fast turnaround! It works! ^_^

@matthew-dean
Copy link
Author

What it passes to the then() function is irrelevant for me. I just need to know when the Promise resolved.

@stipsan
Copy link
Member

stipsan commented Jul 12, 2018

Alright, good news! 😄👍

@matthew-dean
Copy link
Author

One thing that I'm seeing now. Although now an element that doesn't need scrolling returns a promise, it only resolves after the timeout, even though no scrolling happened. I would expect that if no scrolling needs to happen, then it resolves immediately.

@stipsan
Copy link
Member

stipsan commented Jul 17, 2018

Good catch! I'll take care of it right after scroll-into-view/compute-scroll-into-view#17 😄

@stipsan
Copy link
Member

stipsan commented Jul 18, 2018

I made a repro here: codepen.io/stipsan/pen/gjwWXV

It is showing a behavior similar to what you mentioned above 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants