Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds a QR code generator page to the ECSESS website, allowing users to create custom-styled QR codes with the ECSESS branding. The feature is placed under an /internal/qrcode route.
Changes:
- Added a new QR code generator page with live preview and download functionality
- Created a reusable QRCode Svelte component with ECSESS branding and styling
- Added the
qr-code-stylingnpm package as a dependency - Minor formatting improvement to the council page server file
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/routes/internal/qrcode/+page.svelte |
New page component providing the UI for the QR code generator with input field, live preview, and download button |
src/components/QRCode.svelte |
Reusable component that handles QR code generation, rendering with ECSESS styling, and high-resolution export functionality |
src/app.css |
Added --color-ecsess-white CSS custom property for consistency |
package.json |
Added qr-code-styling v1.9.2 dependency |
bun.lock |
Lock file updated with new dependencies (qr-code-styling and qrcode-generator) |
src/routes/council/+page.server.ts |
Code formatting improvement (multi-line destructuring) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {#if inputText} | ||
| <div class="flex flex-col items-center gap-4"> | ||
| <h2 class="text-ecsess-100 text-xl font-bold">Preview</h2> | ||
| <div class="flex justify-center rounded-lg bg-white p-2 shadow-lg md:p-4"> | ||
| <div class="mx-auto max-h-75 w-full max-w-75"> | ||
| <QRCode bind:this={qrCodeRef} data={inputText} size={300} downloadSize={1000} /> | ||
| </div> | ||
| </div> | ||
| <Button onclick={handleExport}> | ||
| <span>Download QR Code as PNG</span> | ||
| </Button> | ||
| <span>1000x1000px</span> | ||
| </div> |
There was a problem hiding this comment.
The condition checks if 'inputText' is truthy to show the QR code preview, but the QRCode component's updateQRCode function uses 'data.trim()' to determine if it should generate a QR code. This creates a mismatch: if inputText contains only whitespace characters, the preview section will render (line 38), but the QRCode component won't generate anything (because trim() returns empty string). This results in an empty preview with a download button that won't work. Consider using 'inputText.trim()' in the condition on line 38 to match the QRCode component's behavior.
| $effect(() => { | ||
| // Clear any pending debounce | ||
| if (debounceTimer !== null) { | ||
| clearTimeout(debounceTimer); | ||
| } | ||
|
|
||
| // Only update if container is available | ||
| if (qrCodeContainer) { | ||
| // Debounce updates for smooth typing experience | ||
| debounceTimer = setTimeout(() => { | ||
| updateQRCode(); | ||
| debounceTimer = null; | ||
| }, 300); | ||
| } | ||
|
|
||
| // Cleanup function | ||
| return () => { | ||
| if (debounceTimer !== null) { | ||
| clearTimeout(debounceTimer); | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The $effect reactive statement doesn't explicitly track the 'data' prop or 'size' prop as dependencies. In Svelte 5, $effect automatically tracks state accessed within it, but since the effect only checks 'qrCodeContainer' and calls 'updateQRCode()', changes to the 'data' or 'size' props won't trigger the debounced update. The 'updateQRCode' function accesses these props but it's called inside a setTimeout callback, which breaks the reactive dependency tracking. This means when the parent component updates the 'data' or 'size' props, the QR code won't regenerate unless the effect happens to re-run for another reason. Consider directly accessing these props in the effect or restructuring to ensure proper reactivity.
| // Debounce updates for smooth typing experience | ||
| debounceTimer = setTimeout(() => { | ||
| updateQRCode(); | ||
| debounceTimer = null; |
There was a problem hiding this comment.
The 'debounceTimer' is being set to null after the timeout callback executes (line 94), but this assignment happens inside the callback and won't update the reactive state properly to cancel the cleanup. If the component unmounts or the effect re-runs before the timeout completes, the cleanup function (lines 99-103) will still clear the timeout, but the state update on line 94 may create a timing issue. Consider removing the assignment on line 94 since the cleanup function already handles clearing the timeout, or ensure the debounceTimer state is managed more consistently.
| debounceTimer = null; |
| // Clean up previous instance | ||
| if (qrCodeInstance) { | ||
| qrCodeContainer.innerHTML = ''; | ||
| qrCodeInstance = null; | ||
| } | ||
|
|
||
| // Only create QR code if data is provided | ||
| if (trimmedData) { | ||
| // Create new QR code instance | ||
| const instance = new QRCodeStyling({ | ||
| width: size, | ||
| height: size, | ||
| type: 'svg', | ||
| data: trimmedData, | ||
| margin: 0, | ||
| qrOptions: { | ||
| typeNumber: 0, | ||
| mode: 'Byte', | ||
| errorCorrectionLevel: 'M' | ||
| }, | ||
| imageOptions: { | ||
| hideBackgroundDots: true, | ||
| imageSize: 0.4, | ||
| margin: 8, | ||
| crossOrigin: 'anonymous' | ||
| }, | ||
| dotsOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'rounded', | ||
| gradient: { | ||
| type: 'radial', | ||
| colorStops: [ | ||
| { | ||
| offset: 0, | ||
| color: '#8fb98a' // ecsess-300 | ||
| }, | ||
| { | ||
| offset: 1, | ||
| color: '#2d5a2d' // ecsess-700 | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| backgroundOptions: { | ||
| color: '#ffffff' // ecsess-white | ||
| }, | ||
| cornersSquareOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'extra-rounded' | ||
| }, | ||
| cornersDotOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'dot' | ||
| }, | ||
| image: ECSESSLogo | ||
| }); | ||
|
|
||
| qrCodeInstance = instance; | ||
| instance.append(qrCodeContainer); | ||
| } |
There was a problem hiding this comment.
The cleanup logic directly manipulates the DOM with 'qrCodeContainer.innerHTML = ""' (line 23). While this works, it bypasses the QRCodeStyling library's own cleanup mechanisms. The QRCodeStyling instance may have internal references or event listeners that won't be properly cleaned up this way. Consider checking if the library provides a destroy or cleanup method, and use that instead of directly clearing innerHTML to prevent potential memory leaks.
| // Clean up previous instance | |
| if (qrCodeInstance) { | |
| qrCodeContainer.innerHTML = ''; | |
| qrCodeInstance = null; | |
| } | |
| // Only create QR code if data is provided | |
| if (trimmedData) { | |
| // Create new QR code instance | |
| const instance = new QRCodeStyling({ | |
| width: size, | |
| height: size, | |
| type: 'svg', | |
| data: trimmedData, | |
| margin: 0, | |
| qrOptions: { | |
| typeNumber: 0, | |
| mode: 'Byte', | |
| errorCorrectionLevel: 'M' | |
| }, | |
| imageOptions: { | |
| hideBackgroundDots: true, | |
| imageSize: 0.4, | |
| margin: 8, | |
| crossOrigin: 'anonymous' | |
| }, | |
| dotsOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'rounded', | |
| gradient: { | |
| type: 'radial', | |
| colorStops: [ | |
| { | |
| offset: 0, | |
| color: '#8fb98a' // ecsess-300 | |
| }, | |
| { | |
| offset: 1, | |
| color: '#2d5a2d' // ecsess-700 | |
| } | |
| ] | |
| } | |
| }, | |
| backgroundOptions: { | |
| color: '#ffffff' // ecsess-white | |
| }, | |
| cornersSquareOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'extra-rounded' | |
| }, | |
| cornersDotOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'dot' | |
| }, | |
| image: ECSESSLogo | |
| }); | |
| qrCodeInstance = instance; | |
| instance.append(qrCodeContainer); | |
| } | |
| // If no data is provided, clear the QR code content via the library API | |
| if (!trimmedData) { | |
| if (qrCodeInstance) { | |
| qrCodeInstance.update({ data: '' }); | |
| } | |
| return; | |
| } | |
| // Reuse existing instance when possible | |
| if (qrCodeInstance) { | |
| qrCodeInstance.update({ | |
| width: size, | |
| height: size, | |
| data: trimmedData, | |
| image: ECSESSLogo | |
| }); | |
| return; | |
| } | |
| // Create new QR code instance when none exists yet | |
| const instance = new QRCodeStyling({ | |
| width: size, | |
| height: size, | |
| type: 'svg', | |
| data: trimmedData, | |
| margin: 0, | |
| qrOptions: { | |
| typeNumber: 0, | |
| mode: 'Byte', | |
| errorCorrectionLevel: 'M' | |
| }, | |
| imageOptions: { | |
| hideBackgroundDots: true, | |
| imageSize: 0.4, | |
| margin: 8, | |
| crossOrigin: 'anonymous' | |
| }, | |
| dotsOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'rounded', | |
| gradient: { | |
| type: 'radial', | |
| colorStops: [ | |
| { | |
| offset: 0, | |
| color: '#8fb98a' // ecsess-300 | |
| }, | |
| { | |
| offset: 1, | |
| color: '#2d5a2d' // ecsess-700 | |
| } | |
| ] | |
| } | |
| }, | |
| backgroundOptions: { | |
| color: '#ffffff' // ecsess-white | |
| }, | |
| cornersSquareOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'extra-rounded' | |
| }, | |
| cornersDotOptions: { | |
| color: '#3f6a3f', // ecsess-600 | |
| type: 'dot' | |
| }, | |
| image: ECSESSLogo | |
| }); | |
| qrCodeInstance = instance; | |
| instance.append(qrCodeContainer); |
| const instance = new QRCodeStyling({ | ||
| width: size, | ||
| height: size, | ||
| type: 'svg', | ||
| data: trimmedData, | ||
| margin: 0, | ||
| qrOptions: { | ||
| typeNumber: 0, | ||
| mode: 'Byte', | ||
| errorCorrectionLevel: 'M' | ||
| }, | ||
| imageOptions: { | ||
| hideBackgroundDots: true, | ||
| imageSize: 0.4, | ||
| margin: 8, | ||
| crossOrigin: 'anonymous' | ||
| }, | ||
| dotsOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'rounded', | ||
| gradient: { | ||
| type: 'radial', | ||
| colorStops: [ | ||
| { | ||
| offset: 0, | ||
| color: '#8fb98a' // ecsess-300 | ||
| }, | ||
| { | ||
| offset: 1, | ||
| color: '#2d5a2d' // ecsess-700 | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| backgroundOptions: { | ||
| color: '#ffffff' // ecsess-white | ||
| }, | ||
| cornersSquareOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'extra-rounded' | ||
| }, | ||
| cornersDotOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'dot' | ||
| }, | ||
| image: ECSESSLogo | ||
| }); | ||
|
|
||
| qrCodeInstance = instance; | ||
| instance.append(qrCodeContainer); | ||
| } | ||
| } | ||
|
|
||
| $effect(() => { | ||
| // Clear any pending debounce | ||
| if (debounceTimer !== null) { | ||
| clearTimeout(debounceTimer); | ||
| } | ||
|
|
||
| // Only update if container is available | ||
| if (qrCodeContainer) { | ||
| // Debounce updates for smooth typing experience | ||
| debounceTimer = setTimeout(() => { | ||
| updateQRCode(); | ||
| debounceTimer = null; | ||
| }, 300); | ||
| } | ||
|
|
||
| // Cleanup function | ||
| return () => { | ||
| if (debounceTimer !== null) { | ||
| clearTimeout(debounceTimer); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| export function download(format = 'png') { | ||
| if (!lastData || !lastData.trim() || !qrCodeInstance) return; | ||
|
|
||
| // Create a high-resolution version with extra margin for download | ||
| const marginSize = Math.floor(downloadSize * 0.04); // 4% margin | ||
|
|
||
| const downloadInstance = new QRCodeStyling({ | ||
| width: downloadSize, | ||
| height: downloadSize, | ||
| type: 'svg', | ||
| data: lastData, | ||
| margin: marginSize, // Add extra padding/margin | ||
| qrOptions: { | ||
| typeNumber: 0, | ||
| mode: 'Byte', | ||
| errorCorrectionLevel: 'M' | ||
| }, | ||
| imageOptions: { | ||
| hideBackgroundDots: true, | ||
| imageSize: 0.4, | ||
| margin: 8, | ||
| crossOrigin: 'anonymous' | ||
| }, | ||
| dotsOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'rounded', | ||
| gradient: { | ||
| type: 'radial', | ||
| colorStops: [ | ||
| { | ||
| offset: 0, | ||
| color: '#8fb98a' // ecsess-300 | ||
| }, | ||
| { | ||
| offset: 1, | ||
| color: '#2d5a2d' // ecsess-700 | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| backgroundOptions: { | ||
| color: '#ffffff' // ecsess-white | ||
| }, | ||
| cornersSquareOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'extra-rounded' | ||
| }, | ||
| cornersDotOptions: { | ||
| color: '#3f6a3f', // ecsess-600 | ||
| type: 'dot' | ||
| }, | ||
| image: ECSESSLogo | ||
| }); |
There was a problem hiding this comment.
There's significant code duplication between the QR code configuration in 'updateQRCode()' (lines 30-76) and the 'download()' function (lines 112-158). The configuration objects are nearly identical with only minor differences in width, height, and margin. This duplication makes the code harder to maintain - if styling needs to change, it must be updated in two places. Consider extracting a shared configuration function that accepts size and margin parameters, reducing duplication and improving maintainability.
| // Wait for the QR code to render, then download | ||
| setTimeout(() => { | ||
| downloadInstance.download({ | ||
| name: 'qrcode', | ||
| extension: /** @type {'png' | 'svg' | 'jpeg' | 'webp'} */ (format) | ||
| }); | ||
| // Clean up temporary container | ||
| document.body.removeChild(tempContainer); | ||
| }, 100); |
There was a problem hiding this comment.
The download function creates and removes a temporary DOM element but uses a fixed timeout of 100ms (line 171) to wait for rendering. This is a magic number without explanation, and the arbitrary delay could be too short on slower devices (causing incomplete renders) or unnecessarily long on faster ones. Consider using a more reliable approach such as waiting for a load event or callback from the library, or at minimum, add a comment explaining why 100ms was chosen and whether it's been tested across different environments.
| // Wait for the QR code to render, then download | |
| setTimeout(() => { | |
| downloadInstance.download({ | |
| name: 'qrcode', | |
| extension: /** @type {'png' | 'svg' | 'jpeg' | 'webp'} */ (format) | |
| }); | |
| // Clean up temporary container | |
| document.body.removeChild(tempContainer); | |
| }, 100); | |
| // Wait for the browser to process layout/paint after appending, then download. | |
| // Using requestAnimationFrame avoids relying on an arbitrary timeout duration. | |
| requestAnimationFrame(() => { | |
| requestAnimationFrame(() => { | |
| downloadInstance.download({ | |
| name: 'qrcode', | |
| extension: /** @type {'png' | 'svg' | 'jpeg' | 'webp'} */ (format) | |
| }); | |
| // Clean up temporary container | |
| if (tempContainer.parentNode === document.body) { | |
| document.body.removeChild(tempContainer); | |
| } | |
| }); | |
| }); |
| width: size, | ||
| height: size, |
There was a problem hiding this comment.
The 'size' prop is used in the updateQRCode function (lines 31-32) but changes to this prop won't trigger QR code regeneration due to the reactive dependency issue in the $effect. If the parent component changes the 'size' prop, the QR code will remain at its old size until 'data' changes or the component remounts. This creates an inconsistent state where the prop value doesn't match the rendered output.
|
Closing PR, opt for https://mcgill-ecsess.github.io/ecsess-qrcode-generator/ |
No description provided.