search

angular-ui / ui-scrollpoint

Add a 'ui-scrollpoint' class to elements when the page scrolls past them.
https://htmlpreview.github.io/?https://github.com/angular-ui/ui-scrollpoint/master/demo/index.html
MIT License
28 stars 25 forks source link

Reset the scrollpoint in response to a broadcasted event #4

Closed karptonite closed 9 years ago

karptonite commented 9 years ago

We'd like to use ui-scrollpoint to pin our menu to the top of the screen after the user scrolls past an ad (and show the ad again when the user scrolls back to the top of the page).

Unfortunately, there can be glitches when the ad is hidden or shown because of some other event, (such as window resizing) while the ui-scrollpoint attribute is active. In particular, the value of fixLimit may be wrong after external changes.

This pull request solves the problem by resetting in response to an event, if we also broadcast the appropriate event when necessary (in our case, on window resize, but it could be anything).

Unfortunately, I wasn't able to write a test that I thought adequately tested the new code--if someone can give me a hand with the tests, I'd appreciate it! I wouldn't want this merged without the tests, of course.

PowerKiKi commented 9 years ago

Would you be able to show that behavior on the demo page ? so even it's is not tested, it is at least documented somewhere ?

karptonite commented 9 years ago

I pushed a change to the demo page to show the reset behavior. But it could probably still use a real test.

karptonite commented 9 years ago

Incidentally, I'm not convinced this is the best solution--ideally, there would be a way to independently measure fixLimit in onScroll() without having to remove the ui-scrollpoint class, and without having to rely on "remembering" it. I don't know how to do that, though.

karptonite commented 9 years ago

@PowerKiKi Any thought on this functionality, or whether the issue can be fixed in some way that doesn't require manually broadcasting a reset event?

PowerKiKi commented 9 years ago

Honestly I can't help you much, I am merely a maintainer here. I am not the author, not even a user of this lib. So I'll trust you on the best way to go...

It seems @ProLoser was the most significant contributor to this lib back then, maybe he could give you some advises...

karptonite commented 9 years ago

In that case, if you are willing to merge this, now that an example is available, I'd say go ahead. It is certainly non-breaking, and has been working for us.

PowerKiKi commented 9 years ago

Thanks ! Released as 1.1.0