Fix for issue #45-- don't try and scroll a position:fixed element into view#64
Fix for issue #45-- don't try and scroll a position:fixed element into view#64asselin wants to merge 2 commits intoiamdustan:masterfrom
Conversation
…ement into view
src/smoothscroll.js
Outdated
| top: parentRects.top, | ||
| behavior: 'smooth' | ||
| }); | ||
| if (w.getComputedStyle(scrollableParent).position != 'fixed') { |
There was a problem hiding this comment.
Thanks for the PR! Now, can you switch this to !==? Strict equality comparison is preferable since its usually faster. Also, add a comment on top of the ìf` statement explaining why the hard check, something like Avoid scrolling fixed positioned parent.
Thanks in advance!
There was a problem hiding this comment.
Sorry for not picking up that this library uses === coding convention, fixed that and added a comment.
|
@asselin nothing to be sorry mna, thanks for your contribution. We are going to make this easier and inform when there's a lint issue beforehand. |
|
Thanks! Sadly, this PR and others are entering in conflict (https://github.com/iamdustan/smoothscroll/pull/73/files) so I'm gonna manually pick up the work of each to simplify the release and be equally fair with everyone since someone would need to resolve the conflicts no matter the other I chose. I'm gonna make a PR and put you as a reviewer so you can check your changes. |
|
hey, i was looking for this. seems @asselin havent reviewed the conflict. :) Cmon publish this please. Thanks. |
3 participants
Issue #45 shows an issue where if the parent element is position:fixed, smoothscroll tries to scroll the body to scroll it to the top of the viewport. position:fixed elements never move of course, so in this case, it shouldn't do anything to the scrollpos of the body.