Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX #14] Add config for IO/initialize in default function #48

Closed
wants to merge 5 commits into from
Closed

[FIX #14] Add config for IO/initialize in default function #48

wants to merge 5 commits into from

Conversation

aretheregods
Copy link

Fixes #14
How about this?

Copy link
Contributor

@lukeed lukeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Few comments, but let's await feedback before making changes 😄

src/index.mjs Outdated
}
});
});
const observerConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this – as is, this just redefines the existing defaults without a way to customize it.

src/index.mjs Outdated
if (toPrefetch.has(url)) prefetcher(url);
}
});
}, observerConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's replace this with a config object that allows the user to directly manipulate the IO settings.

Suggested change
}, observerConfig);
}, options.config || {});

The only drawback here is that it requires the user to know what those options are... but we can easily link to its options in the docs IMO. Plus, I personally prefer to expose native options, rather than reinvent them with new names.

What do you think @addyosmani ?

Copy link
Author

@aretheregods aretheregods Dec 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was gonna do that. But I didn't wanna add something to options without advisement. I'll make that change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to add that I'm in support of exposing native names vs defining our own variants where possible 👍

Thanks for working on this, @aretheregods!

@aretheregods
Copy link
Author

I removed observerConfig, but still need to add the exposed options for options.config.

@aretheregods
Copy link
Author

I added the JSDoc params for each IO property with their standard names.

 @param {Object} options.config - IntersectionObserver configuration options
 @param {Element} options.config.root - The root container in which elements are observed
 @param {string} options.config.rootMargin - Margin added to root before intersection observation testing
 @param {number} options.config.threshold - percentage of observed element in root before IO callback is called

And I set config to an empty object in options and then just passed that to IO setup.

    el: document,
    config: {},
  }, options);
       if (toPrefetch.has(url)) prefetcher(url);
      }
    });
  }, options.config);

Please let me know if this works for you?

@addyosmani
Copy link
Collaborator

As a passing note, it looks like this will need to be rebased against master ♻️

@aretheregods
Copy link
Author

aretheregods commented Dec 17, 2018

@addyosmani I believe that rebasing business might possibly be resolved...

P.S. Have a great holiday, guys

@addyosmani
Copy link
Collaborator

@lukeed I believe this PR managed to address most of our feedback. Was there anything else remaining before you were happy to sign off on it?

@aretheregods sorry to ask again - would you mind rebasing one more time? It's entirely my fault for not circling back to wrap up reviews when you first tried addressing our feedback. Would love to wrap this one up.

*/
export default function (options) {
options = Object.assign({
timeout: 2e3,
priority: false,
timeoutFn: requestIdleCallback,
el: document,
config: {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this line, as new IntersectionObserver(noop, undefined) is still valid 😆

Suggested change
config: {}

@addyosmani
Copy link
Collaborator

@aretheregods a friendly ping. Looks like we are close to this being landable :)

@addyosmani addyosmani closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding rootMargin to the Intersection Observer
3 participants