fix: Correct Portrait Image Scaling (Resolves #10) & Performance Refactoring (v1.0.3)#19
Conversation
- Fixes incorrect scaling for portrait images (main issue). - Prevents infinite images scrolling that was a side effect of the scaling logic.
This comprehensive refactoring pass improved maintainability and reduced the minified bundle size by 1.8%. Key changes include: - Architectural Standardization: Over 40 "magic numbers" were extracted into named constants. - Documentation: Added comprehensive JSDoc to all critical public and internal methods. - Code Deduplication: Consolidated duplicate calibration and opacity management logic into reusable helper functions. - Organization: Introduced clear section headers to improve code navigability.
- Integrated @rollup/plugin-replace and @rollup/plugin-strip for better build artifact cleanup. - Improved handling of CSS class/ID renaming and short name generation for smaller output.
- updated Russian translations for clarity and accuracy; - bumped version to 1.0.3.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical portrait image scaling bug (issue #10) and introduces significant quality-of-life improvements including a dark theme, opening animations, performance refactoring, and build optimizations. The changes span build configuration, core library functionality, demo page styling, and localization.
Key Changes:
- Fixed portrait image scaling to use proper "contain" logic (fully visible by default)
- Added dark theme support to demo page with comprehensive CSS media queries
- Implemented opening animation (scale 0.9 → 1.0) with delayed UI visibility
- Changed navigation from circular to linear (stops at first/last image)
- Refactored 40+ magic numbers into named constants
- Enhanced build tooling with rollup plugins and increased Terser optimization passes
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/spotlight.js | Core fixes: portrait scaling logic, linear navigation, animation timing, refactored constants, added JSDoc, improved calibration cleanup |
| vite.config.js | Build enhancements: added replace/strip plugins, increased Terser passes to 10, custom size reporting, CSS/SVG/HTML optimizers |
| vite.config.safe.js | Simplified build config without advanced optimizations, custom logging, size reporting |
| package.json | Version bump to 1.0.3, added @rollup/plugin-replace and @rollup/plugin-strip dependencies |
| package-lock.json | Dependency lockfile updates for new rollup plugins |
| demo/index.html | Dark theme CSS (240+ lines), changed script path to local copy, improved visual theming, scrollbar hiding |
| demo/languages.js | Updated Russian translations with improved clarity and non-breaking spaces |
| dist/spotlight.min.js | Regenerated minified bundle reflecting all source changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _showOverlay() { | ||
| this.overlay.style.display = 'block'; | ||
| this._lastFocused = document.activeElement; | ||
|
|
There was a problem hiding this comment.
The double requestAnimationFrame call (lines 1633-1634) forces a two-frame delay before showing the overlay, but lacks a comment explaining why this is necessary. While this pattern is sometimes used to ensure layout is complete before animating, consider adding a comment to explain the purpose, as this affects the user experience and could be confusing to future maintainers.
| // Double requestAnimationFrame is used here to ensure that the overlay's | |
| // layout and style changes are fully flushed before adding the 'spot-open' | |
| // class, which triggers the opening animation. This prevents visual glitches | |
| // and ensures a smooth transition. |
| plugins.push( | ||
| strip({ | ||
| include: 'src/**', | ||
| functions: ['console.*', 'assert.*', 'debug*'], |
There was a problem hiding this comment.
Both the @rollup/plugin-strip (line 245) and Terser's drop_console option (line 266) are configured to remove console statements, which is redundant. Since Terser runs later and is more comprehensive, the strip plugin's console removal is unnecessary. Consider removing 'console.*' from the strip plugin's functions array to avoid duplicate work and simplify the build configuration.
| functions: ['console.*', 'assert.*', 'debug*'], | |
| functions: ['assert.*', 'debug*'], |
| gallery_text: | ||
| 'Создавать галереи просто. Оберните картинки в блок с классом <code>.gallery</code>, и Spotlight объединит их в коллекцию.', | ||
| article_mode: 'Режим статьи', | ||
| 'Самый простой способ объеденить изображения в галерею — обернуть картинки в блок с классом <code>.gallery</code>, и Spotlight начнёт их отображать как единую коллекцию.', |
There was a problem hiding this comment.
Typo in Russian text: "объеденить" should be "объединить" (unite/combine).
| 'Самый простой способ объеденить изображения в галерею — обернуть картинки в блок с классом <code>.gallery</code>, и Spotlight начнёт их отображать как единую коллекцию.', | |
| 'Самый простой способ объединить изображения в галерею — обернуть картинки в блок с классом <code>.gallery</code>, и Spotlight начнёт их отображать как единую коллекцию.', |
| prev() { | ||
| const c = this.collections[this.state.collectionIndex]; | ||
| if (!c) { | ||
| if (!c || this.state.itemIndex <= 0) { | ||
| return; | ||
| } | ||
| this.state.itemIndex = | ||
| (this.state.itemIndex - 1 + c.items.length) % c.items.length; | ||
| this.state.itemIndex--; | ||
| this._animateSlide(-1, () => this._loadItem()); | ||
| } | ||
|
|
||
| /** | ||
| * Navigates to the next image in the current collection. | ||
| */ | ||
| next() { | ||
| const c = this.collections[this.state.collectionIndex]; | ||
| if (!c) { | ||
| if (!c || this.state.itemIndex >= c.items.length - 1) { | ||
| return; | ||
| } | ||
| this.state.itemIndex = (this.state.itemIndex + 1) % c.items.length; | ||
| this.state.itemIndex++; | ||
| this._animateSlide(1, () => this._loadItem()); | ||
| } |
There was a problem hiding this comment.
The prev() and next() methods now prevent circular navigation (stopping at the first/last image), but this is a breaking behavioral change from the previous version which wrapped around. This change should be documented in the PR description or release notes, as users may rely on the circular behavior. Consider adding a configuration option to allow users to choose between linear and circular navigation.
| terserOptions: { | ||
| compress: { | ||
| passes: 3, | ||
| passes: 10, |
There was a problem hiding this comment.
Increasing Terser passes from 3 to 10 will significantly increase build time (potentially 3x or more) with diminishing returns on bundle size reduction after 3-5 passes. Unless benchmarks show meaningful improvements, consider reducing this to 5 passes as a more balanced tradeoff between build speed and optimization.
| passes: 10, | |
| passes: 5, |
| const patchLogs = () => { | ||
| const shouldHide = (str) => { | ||
| if (!str) return false; | ||
| const clean = str.replace(/\x1b\[[0-9;]*m/g, ''); |
There was a problem hiding this comment.
Unexpected control character(s) in regular expression: \x1b.
| min.replace(/\\/g, '\\\\').replace(/`/g, '\\`') | ||
| ), | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
Empty block statement.
| } catch {} | |
| } catch (err) { | |
| console.error('Failed to minify HTML:', err); | |
| } |
| `dist/\x1b[36m${fileName}\x1b[0m ` + | ||
| `\x1b[38;5;15m${size(buf.length)} │ gzip: \x1b[1;30m${size(gz(buf))} \x1b[0m\x1b[38;5;15m│ \x1b[38;5;15mbrotli: \x1b[1;30m${size(br(buf))}\x1b[0m` | ||
| ); | ||
| } catch {} |
There was a problem hiding this comment.
Empty block statement.
| } catch {} | |
| } catch (err) { | |
| console.error('Error reporting bundle size:', err); | |
| } |
| const patchLogs = () => { | ||
| const shouldHide = (str) => { | ||
| if (!str) return false; | ||
| const clean = str.replace(/\x1b\[[0-9;]*m/g, ''); |
There was a problem hiding this comment.
Unexpected control character(s) in regular expression: \x1b.
| `dist/\x1b[36m${fileName}\x1b[0m ` + | ||
| `\x1b[38;5;15m${size(buf.length)} │ gzip: \x1b[1;30m${size(gz(buf))} \x1b[0m\x1b[38;5;15m│ \x1b[38;5;15mbrotli: \x1b[1;30m${size(br(buf))}\x1b[0m` | ||
| ); | ||
| } catch {} |
There was a problem hiding this comment.
Empty block statement.
| } catch {} | |
| } catch (err) { | |
| console.error('Error reporting bundle sizes:', err); | |
| } |
Feature/Refactoring: Dark Theme, Performance Enhancements, and Bug Fixes
Closes: #10 (Correct Portrait Image Scaling)
This Pull Request introduces significant improvements in aesthetics, performance, and code quality, most notably by implementing the requested Dark Theme and resolving a critical scaling issue for portrait images.
Key New Features & Enhancements (
feat,build)@rollup/plugin-replaceand@rollup/plugin-strip. This improves artifact cleanup and delivers smaller, more efficient final bundles through better handling of CSS renaming.1.0.3.Bug Fixes (
fix)Code Quality & Refactoring (
refactor)A comprehensive refactoring pass was performed to improve maintainability and reduce the bundle size.
Commit Summary (Chronological Order)
feat: implement dark theme for demo pagebuild: Enhance build script quality and minification efficiencyrefactor: Clean up codebase, enforce standards, and reduce duplicationfeat: implement spotlight opening animationsfix: Corrected image swiping on scaled view when using touchpad gesturesfix: default scaling issue for portrait images