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

Add native lazy loading callback #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

plcossette
Copy link
Contributor

No description provided.

Comment on lines +32 to +35
IMAGE: "c-image",
IMAGE_LAZY_LOADED: "-lazy-loaded",
IMAGE_LAZY_LOADING: "-lazy-loading",
IMAGE_LAZY_ERROR: "-lazy-error",
Copy link
Member

Choose a reason for hiding this comment

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

Respect the existing quotation style and prioritize single quotes.

Suggested change
IMAGE: "c-image",
IMAGE_LAZY_LOADED: "-lazy-loaded",
IMAGE_LAZY_LOADING: "-lazy-loading",
IMAGE_LAZY_ERROR: "-lazy-error",
IMAGE: 'c-image',
IMAGE_LAZY_LOADED: '-lazy-loaded',
IMAGE_LAZY_LOADING: '-lazy-loading',
IMAGE_LAZY_ERROR: '-lazy-error',

Comment on lines +25 to +27
/**
* Trigger lazyload
*/
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous comment given the called function name.

Suggested change
/**
* Trigger lazyload
*/

Comment on lines +22 to +24
/**
* Trigger lazyload
*/
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous comment given the called function name.

Suggested change
/**
* Trigger lazyload
*/

triggerLazyloadCallbacks();
});

this.load.on("loading", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Respect the existing quotation style and prioritize single quotes.

Suggested change
this.load.on("loading", () => {
this.load.on('loading', () => {

Comment on lines +29 to +31
/**
* Remove previous lazyload callbacks
*/
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous comment given the called function name.

Suggested change
/**
* Remove previous lazyload callbacks
*/

Comment on lines +32 to +48
/**
* Observe new scroll elements
*
* @param $newContainer (HTMLElement)
*/
addScrollElements($newContainer) {
this.scroll?.addScrollElements($newContainer)
}

/**
* Unobserve scroll elements
*
* @param $oldContainer (HTMLElement)
*/
removeScrollElements($oldContainer) {
this.scroll?.removeScrollElements($oldContainer)
}
Copy link
Member

Choose a reason for hiding this comment

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

Misaligned block comment syntax and invalid parameter tags.

Suggested change
/**
* Observe new scroll elements
*
* @param $newContainer (HTMLElement)
*/
addScrollElements($newContainer) {
this.scroll?.addScrollElements($newContainer)
}
/**
* Unobserve scroll elements
*
* @param $oldContainer (HTMLElement)
*/
removeScrollElements($oldContainer) {
this.scroll?.removeScrollElements($oldContainer)
}
/**
* Observe new scroll elements
*
* @param {HTMLElement} $newContainer
*/
addScrollElements($newContainer) {
this.scroll?.addScrollElements($newContainer)
}
/**
* Unobserve scroll elements
*
* @param {HTMLElement} $oldContainer
*/
removeScrollElements($oldContainer) {
this.scroll?.removeScrollElements($oldContainer)
}

Copy link
Member

Choose a reason for hiding this comment

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

Out of scope coding style corrections. Ideally, these corrections should be their own commit either in a different pull request or directly on the main branch.

Respect the existing quotation style and prioritize single quotes.

Comment on lines +129 to +154
// https://gomakethings.com/how-to-get-the-closest-parent-element-with-a-matching-selector-using-vanilla-javascript/
const queryClosestParent = ($el, selector) => {
// Element.matches() polyfill
if (!Element.prototype.matches) {
Element.prototype.matches =
Element.prototype.matchesSelector ||
Element.prototype.mozMatchesSelector ||
Element.prototype.msMatchesSelector ||
Element.prototype.oMatchesSelector ||
Element.prototype.webkitMatchesSelector ||
function (s) {
var matches = (
this.document || this.ownerDocument
).querySelectorAll(s),
i = matches.length;
while (--i >= 0 && matches.item(i) !== this) {}
return i > -1;
};
}

// Get the closest matching element
for (; $el && $el !== document; $el = $el.parentNode) {
if ($el.matches(selector)) return $el;
}
return null;
};
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary polyfill removed in f527488 (2022-06) and replaced in 082f3b5 (2021-10) with Element.prototype.closest.

Suggested change
// https://gomakethings.com/how-to-get-the-closest-parent-element-with-a-matching-selector-using-vanilla-javascript/
const queryClosestParent = ($el, selector) => {
// Element.matches() polyfill
if (!Element.prototype.matches) {
Element.prototype.matches =
Element.prototype.matchesSelector ||
Element.prototype.mozMatchesSelector ||
Element.prototype.msMatchesSelector ||
Element.prototype.oMatchesSelector ||
Element.prototype.webkitMatchesSelector ||
function (s) {
var matches = (
this.document || this.ownerDocument
).querySelectorAll(s),
i = matches.length;
while (--i >= 0 && matches.item(i) !== this) {}
return i > -1;
};
}
// Get the closest matching element
for (; $el && $el !== document; $el = $el.parentNode) {
if ($el.matches(selector)) return $el;
}
return null;
};

Copy link
Member

@mcaskill mcaskill Aug 9, 2023

Choose a reason for hiding this comment

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

Use Element.prototype.closest instead of the queryClosestParent polyfill. See comment in html.js

Respect the existing quotation style and prioritize single quotes.

@mcaskill mcaskill changed the title Add natif lazy-load callback Add native lazy loading callback Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants