Skip to content
This repository was archived by the owner on Sep 8, 2020. It is now read-only.

Conversation

@karptonite
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@karptonite
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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 added a commit that referenced this pull request Oct 27, 2015
Reset the scrollpoint in response to a broadcasted event
@PowerKiKi PowerKiKi merged commit 083af20 into angular-ui:master Oct 27, 2015
@PowerKiKi
Copy link
Contributor

Thanks ! Released as 1.1.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants