Conversation
…nd preload directives, implementing lazy loading for images, and documenting optimization strategies.
…bar, and clock, integrating the Snake game into the new desktop environment.
There was a problem hiding this comment.
Pull request overview
This PR represents a comprehensive rework of the BertenX portfolio website, transforming it from a static HTML/CSS/JS site into a modern Vite-based build system with a nostalgic Windows 95 theme. The redesign replaces the previous green-themed modern interface with an interactive desktop environment featuring draggable windows, a taskbar, and retro styling.
Key Changes:
- Migration to Vite build system with single-file HTML output using vite-plugin-singlefile
- Complete UI redesign with Windows 95 aesthetic (window chrome, beveled borders, system fonts)
- Reorganization of source files into
/srcdirectory with ES modules - Enhanced build optimizations including asset inlining and console stripping
Reviewed changes
Copilot reviewed 10 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.js | Main site Vite configuration with single-file plugin and build optimizations |
| vite.config.snake.js | Separate config for snake game build due to MPA limitations |
| package.json | Defines build scripts, dependencies, and package manager version |
| pnpm-lock.yaml | Lockfile for Vite 7.2.4 and vite-plugin-singlefile 2.3.0 |
| src/main.js | New interactive window dragging, taskbar, and clock functionality |
| src/style.css | Complete CSS rewrite with Windows 95 design system |
| src/snake.js | Refactored snake game logic (moved from root) |
| src/snake.css | Snake game styles (moved from root) |
| index.html | Restructured as Windows 95 desktop with draggable windows |
| snake.html | Wrapped snake game in Windows 95 window chrome |
| styles.css | Deleted - replaced by src/style.css |
| script.js | Deleted - replaced by src/main.js |
| README.md | Updated documentation for new build process and tech stack |
| .gitignore | Added Node.js, Vite, and build artifact patterns |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snake: resolve(__dirname, 'snake.html'), | ||
| }, | ||
| }, | ||
| assetsInlineLimit: 100000000, // 100MB |
There was a problem hiding this comment.
The assetsInlineLimit is set to 100MB (100000000 bytes), which is extremely high. This means any asset under 100MB will be inlined as base64, significantly increasing the HTML file size. Consider using a more reasonable limit (e.g., 10KB-50KB) unless there's a specific requirement for inlining very large assets.
| assetsInlineLimit: 100000000, // 100MB | |
| assetsInlineLimit: 51200, // 50KB |
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Snake Game</title> | ||
| <link rel="stylesheet" href="/src/style.css" /> |
There was a problem hiding this comment.
The snake game page imports the main site's CSS (/src/style.css) which includes styles for Windows 95 windows, taskbars, and other portfolio elements. This creates unnecessary CSS bloat for the snake game. Consider creating a separate minimal CSS file for the snake game or using a modular approach to only include necessary styles.
| <link rel="stylesheet" href="/src/style.css" /> | |
| <link rel="stylesheet" href="/src/snake.css" /> |
| document.addEventListener('mousemove', (e) => { | ||
| if (!isDragging) return; | ||
|
|
||
| const dx = e.clientX - startX; | ||
| const dy = e.clientY - startY; | ||
|
|
||
| win.style.left = `${initialLeft + dx}px`; | ||
| win.style.top = `${initialTop + dy}px`; | ||
| }); | ||
|
|
||
| document.addEventListener('mouseup', () => { | ||
| isDragging = false; | ||
| }); |
There was a problem hiding this comment.
The mousemove and mouseup event listeners are added to the document for every window in the loop, which creates duplicate listeners. These should be registered once outside the loop, or the dragging logic should be refactored to avoid creating multiple identical listeners.
Consider moving the dragging logic to use event delegation or ensure listeners are properly cleaned up.
| - Uses Vite for fast development and optimized production builds | ||
| - Separate build configurations for main site and snake game due to vite-plugin-singlefile MPA limitations | ||
| - All assets are inlined in production builds for single-file deployment | ||
| - Development server runs on port 3000 to avoid service worker conflicts |
There was a problem hiding this comment.
The documentation mentions "Development server runs on port 3000 to avoid service worker conflicts", but this conflicts with the actual port configuration (5174) in vite.config.js. Either the documentation or the configuration should be updated to match.
| - Development server runs on port 3000 to avoid service worker conflicts | |
| - Development server runs on port 5174 to avoid service worker conflicts |
| main: resolve(__dirname, 'index.html'), | ||
| }, | ||
| }, | ||
| assetsInlineLimit: 100000000, // 100MB |
There was a problem hiding this comment.
The assetsInlineLimit is set to 100MB (100000000 bytes), which is extremely high. This means any asset under 100MB will be inlined as base64, significantly increasing the HTML file size. Consider using a more reasonable limit (e.g., 10KB-50KB) unless there's a specific requirement for inlining very large assets.
| assetsInlineLimit: 100000000, // 100MB | |
| assetsInlineLimit: 10240, // 10KB |
| } else { | ||
| // If already active and on top, minimize (hide) | ||
| // Check if it's the top window | ||
| if (parseInt(targetWin.style.zIndex) === highestZ) { |
There was a problem hiding this comment.
The logic for determining if a window is "on top" uses string-to-number conversion of zIndex which could fail if zIndex is not set. Consider using parseInt(targetWin.style.zIndex || '0') or tracking the active window in a more robust way to avoid potential NaN comparisons.
| if (parseInt(targetWin.style.zIndex) === highestZ) { | |
| if (parseInt(targetWin.style.zIndex || '0') === highestZ) { |
| ### Prerequisites | ||
|
|
||
| No build tools or dependencies required! This is a simple static website. | ||
| - [Node.js](https://nodejs.org/) (v18 or higher) |
There was a problem hiding this comment.
The documentation specifies Node.js v18 or higher as a prerequisite, but vite.config.js uses Vite 7.2.4 which according to the pnpm-lock.yaml requires Node.js ^20.19.0 || >=22.12.0. The documentation should be updated to reflect the actual minimum Node.js version requirement (v20.19.0 or v22.12.0+).
| - [Node.js](https://nodejs.org/) (v18 or higher) | |
| - [Node.js](https://nodejs.org/) (v20.19.0 or v22.12.0+) |
| @media (max-width: 768px) { | ||
| .hamburger { | ||
| display: flex; | ||
| } | ||
|
|
||
| .nav-menu { | ||
| position: fixed; | ||
| left: -100%; | ||
| top: 70px; | ||
| flex-direction: column; | ||
| background: rgba(20, 69, 47, 0.98); | ||
| width: 100%; | ||
| text-align: center; | ||
| transition: 0.3s; | ||
| padding: 2rem 0; | ||
| } | ||
|
|
||
| .nav-menu.active { | ||
| left: 0; | ||
| } | ||
|
|
||
| .hero-title { | ||
| font-size: 2.5rem; | ||
| } | ||
|
|
||
| .hero-buttons { | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .projects-grid { | ||
| grid-template-columns: 1fr; | ||
| } | ||
|
|
||
| .project-links { | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .social-icons { | ||
| flex-wrap: wrap; | ||
| } | ||
| } |
There was a problem hiding this comment.
The responsive design media query contains styles for elements that don't exist in the new Windows 95 design (.hamburger, .nav-menu, .hero-title, .hero-buttons, .project-links). These appear to be leftover from the old design and should be removed or updated to match the new window-based layout.
| pnpm run dev | ||
| ``` | ||
|
|
||
| The site will be available at `http://localhost:3000` |
There was a problem hiding this comment.
The documentation states the site will be available at http://localhost:3000, but vite.config.js line 20 specifies port 5174. This discrepancy should be corrected to match the actual configured port.
| The site will be available at `http://localhost:3000` | |
| The site will be available at `http://localhost:5174` |
No description provided.