-
Notifications
You must be signed in to change notification settings - Fork 333
Support {block: 'end'} #39
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
Conversation
|
This isn't quite right yet - there's an issue I'm still trying to track down - but I'd be surprised if the final implementation was very different. |
|
The recursive call to |
|
OK, figured it out. I suspect this didn't (and still doesn't) work if you nest scrollable elements arbitrarily deep, but that's a terrible UX anyways and practically nobody does it. |
|
oh dang. This looks solid, @DanielHeath. Sorry for neglecting it for so long. Would you be alright picking this back up after #54 lands? I’ll get you a proper review then if you’re game. |
|
I'm about to go on paternity leave (woot) so probably won't be about much. There's a bug on chrome >= 56 that stops it cleaning up after itself properly - after scrolling to something it breaks further attempts to scroll. Haven't had time to investigate yet. |
|
Oh awesome! Congratulations, @DanielHeath! Enjoy dadmode! 😄 |
|
@iamdustan @DanielHeath guys, could you actualise this PR and merge it? It's a crucial feature for me to use. UPD: I created a PR #88 based on the most recent |
I've broken my work into four logically separate commits for easy review.
The test harness renders a sequence of iframes with different setups. I found it invaluable when comparing behavior across browsers.
Adding support for overriding browser builtins made comparative testing much easier. I also plan to use it in my application because firefox's implementation has bugs.
Renaming
shouldBailOuttoscrollIsInstantmakes theblock: endstuff read much better.The final change actually implements the
block: 'end'scroll behavior.