Invalidation Issue

[erasaur]

If I scroll away from the hashed element and, say, load some more documents due to infinite scrolling, onAfterAction is invalidated and the scroll shoots back up to the hashed element. Perhaps if the hash has not changed, don't scroll twice to it?

[pauldowman]

@erasaur Thanks for pointing this out (Sorry for the slow reply!)

I think the fix is that we should be using onRun instead of onAfterAction. I'll try to do this in the next couple of days, unless anyone hits us with a PR first. :-)

[pauldowman]

8e876df and version 0.0.7 should fix this, try it out and let me know if you're having any problems.

[erasaur]

I guess the reason I didn't suggest onRun is that onAfterAction has some added advantages. For one, onRun stops working after a hot-code reload, whereas onAfterAction doesn't. onAfterAction also works even if you're not routing to a new page, e.g scrolling to another hash on the same route after the original page load.

[pauldowman]

I'm pretty sure I tested it with scrolling to another hash on the same page, but if I broke that then you're right, that would be a problem. I'll double-check tomorrow.

But regarding re-running after a hot code push, I'm not sure I'd want that, especially not at the cost of added complexity elsewhere.

I think this has all the right behavior, let me know if you disagree.

[erasaur]

Oh I haven't tested it with this package yet. But I have implemented this behavior with onRun in the past and the result is that scrolling to hash still works, but the scroll doesn't animate as you would expect (so in essence the scrolling happens because of IR itself, not this package). Maybe something has since changed, I can't be sure because I haven't looked into it yet.

What I meant about the hot-code reload is that after it happens once, onRun hooks stop firing until you hard refresh the page (so even when you navigate to another route with a hash, there won't be any scrolling). This is somewhat unexpected but intended behavior in IR. I wasn't suggesting that we should automatically refresh the page on hot code push. Again, I haven't tested with this package so I apologize in advance if it indeed works for you! :-)

[pauldowman]

Oh, weird, you're right, this totally breaks it for all route changes after hot code push. The docs say "Note that this hook won't run again if the route is reloaded via hot code push", and I though that meant that it wouldn't be re-run for the current route.

And to the other point: yes, it doesn't fire when clicking on a link to an id on the same page, so doesn't animate. But honestly the original behavior of animating when scrolling to a hash on the same page was an unintended side-effect, the purpose of this package was just to fix the behaviour when changing pages, so I think this is actually good.

But anyway, the first issue is kind of bad so I'm re-opening this.

[pauldowman]

There's an open Iron Router issue about this, it's a bug that onRun never runs again after a hot code push.

So assuming that gets fixed I think onRun is the right way to do this.