Enabling intersection observer options.

[TheodorTomas]

Passing down intersection observer options via the args object.

[tazsingh]

Thanks! Can you also add a test for this? Also would be easier for me to gauge the desired API.

If I’m not mistaken, the options could also be undefined. Not sure exactly how all IntersectionObservers across browsers react to that. I’m assuming it’s fine but would be good to double check as well as add a test, or ensure it’s not passed if undefined.

Annoyingly I can’t log into my Github account and can only comment via email due to the 2FA code being sent to a different phone number 😅 If I can think of a solution by the time this PR looks good, I’ll be happy to merge but may have to wait until I’m back at my computer...

Sent from my iPhone

On Nov 1, 2018, at 5:08 PM, Theodór Tómas notifications@github.com wrote:

Passing down intersection observer options via the args object.

You can view, comment on, or merge this pull request online at:

https://github.com/stratiformltd/react-loadable-visibility/pull/17

Commit Summary

Enabling intersection observer options. File Changes

M src/createLoadableVisibilityComponent.js (23) Patch Links:

https://github.com/stratiformltd/react-loadable-visibility/pull/17.patch https://github.com/stratiformltd/react-loadable-visibility/pull/17.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[TheodorTomas]

Yeah I will add some tests. The intersection observer API states that if any parameter is not supplied it uses default values for each option parameter. I also tested this in chrome and it seems to work. I could add a function default(i.e. observerOptions = {}) to insure this will work across all browsers.

If it is not too much hassle it would be nice to have this published sooner rather than later since I am relying on this code for a project I am working on, but I understand if it has to wait.

I will update the PR with unit tests in a short while.

On Thu, Nov 1, 2018 at 9:20 PM Tasveer Singh notifications@github.com wrote:

Thanks! Can you also add a test for this? Also would be easier for me to gauge the desired API.

If I’m not mistaken, the options could also be undefined. Not sure exactly how all IntersectionObservers across browsers react to that. I’m assuming it’s fine but would be good to double check as well as add a test, or ensure it’s not passed if undefined.

Annoyingly I can’t log into my Github account and can only comment via email due to the 2FA code being sent to a different phone number 😅 If I can think of a solution by the time this PR looks good, I’ll be happy to merge but may have to wait until I’m back at my computer...

Sent from my iPhone

On Nov 1, 2018, at 5:08 PM, Theodór Tómas notifications@github.com wrote:

Passing down intersection observer options via the args object.

You can view, comment on, or merge this pull request online at:

https://github.com/stratiformltd/react-loadable-visibility/pull/17

Commit Summary

Enabling intersection observer options. File Changes

M src/createLoadableVisibilityComponent.js (23) Patch Links:

https://github.com/stratiformltd/react-loadable-visibility/pull/17.patch https://github.com/stratiformltd/react-loadable-visibility/pull/17.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stratiformltd/react-loadable-visibility/pull/17#issuecomment-435173573, or mute the thread https://github.com/notifications/unsubscribe-auth/ACfqFpGpkhk9cD7Gd6sJJUGqGDEFmSHGks5uq1efgaJpZM4YI2dt .

-- Theodór Tómas Theodórsson Software Engineer

https://is.linkedin.com/pub/theod%C3%B3r-t%C3%B3mas-theod%C3%B3rsson/b5/a14/2 www.theodortomas.com

[tazsingh]

Yeah the default argument sounds good 👌🏽

Thinking about how to merge this, even if I did, I couldn’t publish to npm without my computer. Maybe let’s get this PR into a good state and run off your branch until then? Sorry for the inconvenience

Sent from my iPhone

On Nov 1, 2018, at 5:33 PM, Theodór Tómas notifications@github.com wrote:

Yeah I will add some tests. The intersection observer API states that if any parameter is not supplied it uses default values for each option parameter. I also tested this in chrome and it seems to work. I could add a function default(i.e. observerOptions = {}) to insure this will work across all browsers.

If it is not too much hassle it would be nice to have this published sooner rather than later since I am relying on this code for a project I am working on, but I understand if it has to wait.

I will update the PR with unit tests in a short while.

On Thu, Nov 1, 2018 at 9:20 PM Tasveer Singh notifications@github.com wrote:

Thanks! Can you also add a test for this? Also would be easier for me to gauge the desired API.

If I’m not mistaken, the options could also be undefined. Not sure exactly how all IntersectionObservers across browsers react to that. I’m assuming it’s fine but would be good to double check as well as add a test, or ensure it’s not passed if undefined.

Annoyingly I can’t log into my Github account and can only comment via email due to the 2FA code being sent to a different phone number 😅 If I can think of a solution by the time this PR looks good, I’ll be happy to merge but may have to wait until I’m back at my computer...

Sent from my iPhone

On Nov 1, 2018, at 5:08 PM, Theodór Tómas notifications@github.com wrote:

Passing down intersection observer options via the args object.

You can view, comment on, or merge this pull request online at:

https://github.com/stratiformltd/react-loadable-visibility/pull/17

Commit Summary

Enabling intersection observer options. File Changes

M src/createLoadableVisibilityComponent.js (23) Patch Links:

https://github.com/stratiformltd/react-loadable-visibility/pull/17.patch https://github.com/stratiformltd/react-loadable-visibility/pull/17.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stratiformltd/react-loadable-visibility/pull/17#issuecomment-435173573, or mute the thread https://github.com/notifications/unsubscribe-auth/ACfqFpGpkhk9cD7Gd6sJJUGqGDEFmSHGks5uq1efgaJpZM4YI2dt .

-- Theodór Tómas Theodórsson Software Engineer

https://is.linkedin.com/pub/theod%C3%B3r-t%C3%B3mas-theod%C3%B3rsson/b5/a14/2 www.theodortomas.com — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[TheodorTomas]

Hey Tasveer, I updated my PR with the changes we discussed along with unit tests. If you have time to review that would be great. If you have any comments on the PR don't hesitate to mention them.

Thanks again

On Thu, Nov 1, 2018 at 9:52 PM Tasveer Singh notifications@github.com wrote:

Yeah the default argument sounds good 👌🏽

Thinking about how to merge this, even if I did, I couldn’t publish to npm without my computer. Maybe let’s get this PR into a good state and run off your branch until then? Sorry for the inconvenience

Sent from my iPhone

On Nov 1, 2018, at 5:33 PM, Theodór Tómas notifications@github.com wrote:

Yeah I will add some tests. The intersection observer API states that if any parameter is not supplied it uses default values for each option parameter. I also tested this in chrome and it seems to work. I could add a function default(i.e. observerOptions = {}) to insure this will work across all browsers.

If it is not too much hassle it would be nice to have this published sooner rather than later since I am relying on this code for a project I am working on, but I understand if it has to wait.

I will update the PR with unit tests in a short while.

On Thu, Nov 1, 2018 at 9:20 PM Tasveer Singh notifications@github.com wrote:

Thanks! Can you also add a test for this? Also would be easier for me to gauge the desired API.

If I’m not mistaken, the options could also be undefined. Not sure exactly how all IntersectionObservers across browsers react to that. I’m assuming it’s fine but would be good to double check as well as add a test, or ensure it’s not passed if undefined.

Annoyingly I can’t log into my Github account and can only comment via email due to the 2FA code being sent to a different phone number 😅 If I can think of a solution by the time this PR looks good, I’ll be happy to merge but may have to wait until I’m back at my computer...

Sent from my iPhone

On Nov 1, 2018, at 5:08 PM, Theodór Tómas notifications@github.com wrote:

Passing down intersection observer options via the args object.

You can view, comment on, or merge this pull request online at:

https://github.com/stratiformltd/react-loadable-visibility/pull/17

Commit Summary

Enabling intersection observer options. File Changes

M src/createLoadableVisibilityComponent.js (23) Patch Links:

https://github.com/stratiformltd/react-loadable-visibility/pull/17.patch

https://github.com/stratiformltd/react-loadable-visibility/pull/17.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/stratiformltd/react-loadable-visibility/pull/17#issuecomment-435173573 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ACfqFpGpkhk9cD7Gd6sJJUGqGDEFmSHGks5uq1efgaJpZM4YI2dt

.

-- Theodór Tómas Theodórsson Software Engineer

< https://is.linkedin.com/pub/theod%C3%B3r-t%C3%B3mas-theod%C3%B3rsson/b5/a14/2

www.theodortomas.com — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stratiformltd/react-loadable-visibility/pull/17#issuecomment-435182934, or mute the thread https://github.com/notifications/unsubscribe-auth/ACfqFihcn8xjy58MrsfUmPlIXij--zSbks5uq17ygaJpZM4YI2dt .

-- Theodór Tómas Theodórsson Software Engineer

https://is.linkedin.com/pub/theod%C3%B3r-t%C3%B3mas-theod%C3%B3rsson/b5/a14/2 www.theodortomas.com

[tazsingh]

Hey sorry for the delay. I'm just getting back from vacation. Will be able to take a deeper look at this and the other issues on Monday. Will check back in then.

[tazsingh]

@TheodorTomas Sorry again for taking so long. Back at the computer now.

Taking a fresh look at this PR, I am unsure of the proposed implementation only because the IntersectionObserver is set up once globally. However, the observerOptions can be passed into each call to react-loadable-visibility but only the 1st mounted loadable will actually set up the IntersectionObserver.

Instead I'd like to propose a different approach that I'd like to hear your thoughts on:

import { provideIntersectionObserverOptions } from 'react-loadable-visibility'

// This is called as many times as you like, but affects all the loadables.
// I think it's a bit more obvious in this case that it's a global call and
// we can continue with this implementation until someone needs a
// more fine-grained approach of a per-loadable IntersectionObserver
// with its own options.
provideIntersectionObserverOptions({
  whatever: 'option'
})

What do you think?

[jasonbiondo]

@tazsingh is there a better solution for this now? It seems like this never got merged and I would like to be able to setup a threshold for when to have the observer kick on.