-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(thumbnail): Lazy load thumbnails #1720
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
base: master
Are you sure you want to change the base?
Conversation
Load thumbnails only when visible using the Intersection Observer API
WalkthroughA debugging console.log statement was removed from the utility file. The thumbnail plugin was updated to support lazy loading of thumbnail images via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Thumbnail
participant IntersectionObserver
User->>Thumbnail: Initialize with settings (thumbnailLazyLoad)
Thumbnail->>IntersectionObserver: Create observer (if lazy loading enabled)
loop For each thumbnail
Thumbnail->>Thumbnail: getThumbHtml()
alt thumbnailLazyLoad enabled
Thumbnail->>Thumbnail: Set img src to placeholder, data-src to real URL
Thumbnail->>IntersectionObserver: Observe img element
else
Thumbnail->>Thumbnail: Set img src to real URL
end
end
IntersectionObserver->>Thumbnail: On img in viewport
Thumbnail->>Thumbnail: Replace src with data-src, remove lazy class, unobserve
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/plugins/thumbnail/lg-thumbnail.ts (1)
531-539: Add observer cleanup to prevent memory leaks.The
destroymethod should disconnect the IntersectionObserver to ensure proper cleanup and prevent potential memory leaks.destroy(): void { if (this.settings.thumbnail) { + this.observer.disconnect(); this.$LG(window).off(`.lg.thumb.global${this.core.lgId}`); this.core.LGel.off('.lg.thumb'); this.core.LGel.off('.thumb'); this.$thumbOuter.remove(); this.core.outer.removeClass('lg-has-thumb'); } }
🧹 Nitpick comments (1)
src/plugins/thumbnail/lg-thumbnail.ts (1)
41-50: Core logic is solid, but consider enhancements.The IntersectionObserver implementation correctly handles lazy loading. Consider these optional improvements:
- Add error handling for failed image loads
- Add configurable threshold/rootMargin options
- Use arrow function for consistency with modern JavaScript patterns
Optional enhanced implementation:
- this.observer = new IntersectionObserver(function (entries, observer) { - entries.forEach(function (entry) { - if (entry.isIntersecting) { - const lazyImage = <HTMLImageElement>entry.target; - lazyImage.src = <string>lazyImage.dataset.src; - lazyImage.classList.remove("lg-lazy-thumb"); - observer.unobserve(lazyImage); - } - }); - }); + this.observer = new IntersectionObserver((entries, observer) => { + entries.forEach((entry) => { + if (entry.isIntersecting) { + const lazyImage = <HTMLImageElement>entry.target; + const src = lazyImage.dataset.src; + if (src) { + lazyImage.src = src; + lazyImage.classList.remove('lg-lazy-thumb'); + observer.unobserve(lazyImage); + } + } + }); + }, { rootMargin: '50px' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lg-utils.ts(0 hunks)src/plugins/thumbnail/lg-thumbnail-settings.ts(2 hunks)src/plugins/thumbnail/lg-thumbnail.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/lg-utils.ts
🧰 Additional context used
🪛 ESLint
src/plugins/thumbnail/lg-thumbnail.ts
[error] 46-46: Replace "lg-lazy-thumb" with 'lg-lazy-thumb'
(prettier/prettier)
[error] 453-453: 'placeholder' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 453-453: Replace ·"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC" with ⏎················'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC'
(prettier/prettier)
[error] 454-454: Replace "lg-lazy-thumb" with 'lg-lazy-thumb'
(prettier/prettier)
🔇 Additional comments (4)
src/plugins/thumbnail/lg-thumbnail-settings.ts (2)
88-91: LGTM! Well-designed API addition.The new
thumbnailLazyLoadsetting is properly documented and follows the existing naming conventions in the interface.
115-115: Good default value choice.Setting
thumbnailLazyLoadtofalseby default ensures backward compatibility and opt-in behavior for the new lazy loading feature.src/plugins/thumbnail/lg-thumbnail.ts (2)
35-35: LGTM! Clean property declaration.The IntersectionObserver property is properly typed and positioned with other private properties.
470-475: LGTM! Proper observer integration.The integration with the IntersectionObserver is well-implemented with proper null checking and conditional logic based on the setting.
| if (this.settings.thumbnailLazyLoad) { | ||
| let placeholder = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC"; | ||
| img.className = "lg-lazy-thumb"; | ||
| img.setAttribute('data-src', thumbImg); | ||
| img.src = placeholder; | ||
| } else { | ||
| img.src = thumbImg; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix formatting and const usage issues.
The lazy loading logic is correct, but address these static analysis issues:
- if (this.settings.thumbnailLazyLoad) {
- let placeholder = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC";
- img.className = "lg-lazy-thumb";
+ if (this.settings.thumbnailLazyLoad) {
+ const placeholder =
+ 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC';
+ img.className = 'lg-lazy-thumb';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.settings.thumbnailLazyLoad) { | |
| let placeholder = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC"; | |
| img.className = "lg-lazy-thumb"; | |
| img.setAttribute('data-src', thumbImg); | |
| img.src = placeholder; | |
| } else { | |
| img.src = thumbImg; | |
| } | |
| if (this.settings.thumbnailLazyLoad) { | |
| const placeholder = | |
| 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC'; | |
| img.className = 'lg-lazy-thumb'; | |
| img.setAttribute('data-src', thumbImg); | |
| img.src = placeholder; | |
| } else { | |
| img.src = thumbImg; | |
| } |
🧰 Tools
🪛 ESLint
[error] 453-453: 'placeholder' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 453-453: Replace ·"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC" with ⏎················'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsQAAA7EAZUrDhsAAAANSURBVBhXYzh8+PB/AAffA0nNPuCLAAAAAElFTkSuQmCC'
(prettier/prettier)
[error] 454-454: Replace "lg-lazy-thumb" with 'lg-lazy-thumb'
(prettier/prettier)
🤖 Prompt for AI Agents
In src/plugins/thumbnail/lg-thumbnail.ts around lines 452 to 459, replace the
mutable variable 'placeholder' with a 'const' since its value does not change,
and fix any inconsistent indentation or spacing to improve code formatting.
Ensure the assignment of className, attributes, and src properties follows
consistent style conventions.
Load thumbnails only when visible using the Intersection Observer API.
Solves #965
Summary by CodeRabbit
New Features
Bug Fixes