search

vvo / in-viewport

Get a callback when any element becomes visible in a viewport (body or custom viewport)
MIT License
276 stars 24 forks source link

Patch to add a watch & dispose API #10

Closed tzi closed 9 years ago

tzi commented 9 years ago

Hi @vvo!

Since I've seen the open issue and I like this project, I would like to try to provide a watch / dispose API.

I had to make some code reorganization, so you probably won't like it :( I made 3 commits, you should probably review one by one, it would be easier:

  1. Add a test for the dispose API
  2. Regroup watches & watching variables and add an API over it to make the code more readable & maintainable. This was not mandatory, but it simplify a lot the next reorganization.
  3. Add a watch & dispose API when isInViewport is called with a callback.

There is no hurry with this PR. I'm available to help if you're interested in this feature.

Cheers, Thomas.

vvo commented 9 years ago

This is really really good!

So to recap we now have a way to stop a watcher and if the inViewport user drops the reference to a watcher then it's garbage collected right?

Thoughts @ryanflorence?

vvo commented 9 years ago
tzi commented 9 years ago

Good catch, I added a short documentation and example in the README file.

Cheers, Thomas.

vvo commented 9 years ago

@tzi let me know what you think about remote.. otherwise Ill merge this tomorrow.

tzi commented 9 years ago

I think remote is more explicit than accessor. I updated the PR. It's ready for merge ;)

vvo commented 9 years ago

So @tzi I merged into master but please can you have a look because the test fails. I rewrote it so that it runs but still the expected behavior of dispose does not works.

When using .dispose() and scrolling, the visible callback is called.

Can you have a look? You can do describe.only() in the dispose test.

Thanks

vvo commented 9 years ago

ok @tzi forget about it, I sorted it out. But I still think you should try to run the test while writing them :D

As easy as npm run dev. I wonder why you did not see/try?

tzi commented 9 years ago

Sorry for the troubles :(

I ran the tests manually and automatically. I even ran on saucelabs sometimes.

I'm sure my test worked and everything was green. But in fact, now there is only 9 tests run instead of 45. I don't know since when...

Thanks for merging & fixing. Thomas.

vvo commented 9 years ago

Your test was not even running, you were mislead by the green status of other tests and the fact that zuul should have errored since the beforeEach never finished.

no big deal, it's a matter of tooling :)

vvo commented 9 years ago

I fixed all of this in the 3.4.0

Thanks again for great contribution

tzi commented 9 years ago

Thanks for your understanding!

Do not hesitate to ask if you need help on this library, since I'm started to handle the code... and soon the tooling :P

Cheers, Thomas.

vvo commented 9 years ago

It's in a pretty good state right now since your help. Only concerned about the tests not passing in saucelabs for some browsers.

The mutation observer test is unstable, dunno why. The library is working but the badge is red :)