feat(ui-templates): let it snow! #33804
Conversation
|
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughThe loading template is enhanced with a conditional snow animation effect. A CSS media query disables the effect when reduced motion is preferred. A new script adds runtime snow particle rendering to a full-window canvas, respecting user preferences and hemisphere-specific seasonal logic (October–December and January for the Northern Hemisphere; May–July for the Southern Hemisphere). The implementation includes canvas resizing, per-particle physics with wind and gravity, a localStorage-persisted toggle button for manual control, and visual effects such as opacity transitions and blur filtering. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/schema/src/config/experimental.ts (1)
231-242: Consider case-insensitive environment variable check.The environment variable check for
NUXT_SNOWis case-sensitive and only recognises the exact string'false'. Users might try'FALSE','False', or'0'expecting them to work.Consider a more robust check:
snowEffect: { $resolve: (val: unknown) => { if (typeof val === 'boolean') { return val } // Check NUXT_SNOW environment variable - if (process.env.NUXT_SNOW === 'false') { + if (process.env.NUXT_SNOW?.toLowerCase() === 'false') { return false } return true }, },packages/ui-templates/templates/loading/index.html (1)
126-127: Consider removing the resize event listener when not needed.The resize event listener remains attached even though the loading screen is typically short-lived. Whilst this is unlikely to cause issues in practice given the transient nature of the loading screen, it's good practice to clean up event listeners.
Consider wrapping the listener in a cleanup function if the loading screen is ever reused or persists longer than expected. However, given this is a loading screen that will be replaced when the app loads, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/3.guide/5.going-further/1.experimental-features.md(1 hunks)packages/schema/src/config/dev.ts(1 hunks)packages/schema/src/config/experimental.ts(2 hunks)packages/schema/src/types/schema.ts(2 hunks)packages/ui-templates/templates/loading/index.html(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/schema/src/config/experimental.tspackages/schema/src/config/dev.tspackages/schema/src/types/schema.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/schema/src/config/experimental.tspackages/schema/src/config/dev.tspackages/schema/src/types/schema.ts
🧠 Learnings (1)
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
Repo: nuxt/nuxt PR: 26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/schema/src/config/dev.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: code
🔇 Additional comments (5)
docs/3.guide/5.going-further/1.experimental-features.md (1)
612-633: LGTM!The documentation clearly describes the feature, including the seasonal behaviour, accessibility considerations, and configuration options. The month ranges accurately match the implementation.
packages/schema/src/types/schema.ts (2)
979-979: LGTM!The
loadingTemplatesignature correctly includes the optionalsnowEffectparameter, maintaining backwards compatibility whilst enabling the new feature.
1474-1484: LGTM!The type definition and JSDoc are clear and comprehensive, accurately describing the feature behaviour and configuration options.
packages/ui-templates/templates/loading/index.html (2)
44-48: LGTM!Properly respects the
prefers-reduced-motionaccessibility setting by hiding the snow canvas. This ensures users who have requested reduced motion won't see the animation.
142-156: Particle recreation logic is efficient.The particle array recreation only occurs when
recreateParticlesis true (triggered by resize), which is an efficient approach that avoids unnecessary allocations during the animation loop.
packages/schema/src/config/dev.ts
Outdated
| loadingTemplate: { | ||
| $resolve: async (_val, get) => { | ||
| const experimental = await get('experimental') | ||
| const snowEffect = experimental.snowEffect | ||
| return (data: { loading?: string }) => { | ||
| const html = loadingTemplate(data) | ||
| // Replace snow effect placeholder with actual config value | ||
| return html.replace(/__SNOW_EFFECT_ENABLED__/g, String(snowEffect)) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Verify type signature matches schema definition.
The returned function's type signature declares data: { loading?: string }, but the schema definition at packages/schema/src/types/schema.ts:979 specifies loadingTemplate: (data: { loading?: string, snowEffect?: boolean }) => string. Whilst the current implementation doesn't use snowEffect from the data parameter (it reads it from experimental config instead), the type should match the schema for consistency.
Consider updating the type signature:
$resolve: async (_val, get) => {
const experimental = await get('experimental')
const snowEffect = experimental.snowEffect
- return (data: { loading?: string }) => {
+ return (data: { loading?: string, snowEffect?: boolean }) => {
const html = loadingTemplate(data)
// Replace snow effect placeholder with actual config value
return html.replace(/__SNOW_EFFECT_ENABLED__/g, String(snowEffect))
}
},📝 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.
| loadingTemplate: { | |
| $resolve: async (_val, get) => { | |
| const experimental = await get('experimental') | |
| const snowEffect = experimental.snowEffect | |
| return (data: { loading?: string }) => { | |
| const html = loadingTemplate(data) | |
| // Replace snow effect placeholder with actual config value | |
| return html.replace(/__SNOW_EFFECT_ENABLED__/g, String(snowEffect)) | |
| } | |
| }, | |
| }, | |
| loadingTemplate: { | |
| $resolve: async (_val, get) => { | |
| const experimental = await get('experimental') | |
| const snowEffect = experimental.snowEffect | |
| return (data: { loading?: string, snowEffect?: boolean }) => { | |
| const html = loadingTemplate(data) | |
| // Replace snow effect placeholder with actual config value | |
| return html.replace(/__SNOW_EFFECT_ENABLED__/g, String(snowEffect)) | |
| } | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In packages/schema/src/config/dev.ts around lines 11 to 21, the returned
function currently types its parameter as data: { loading?: string } but the
project schema at packages/schema/src/types/schema.ts:979 expects
loadingTemplate: (data: { loading?: string, snowEffect?: boolean }) => string;
update the returned function's parameter type to include snowEffect?: boolean
(or remove the explicit annotation so TypeScript infers the correct type from
the schema) so the signature matches the schema while keeping the implementation
that reads snowEffect from experimental config unchanged.
CodSpeed Performance ReportMerging #33804 will not alter performanceComparing Summary
|
|
This might be the most important PR yet! 💗 |
There was a problem hiding this comment.
Thanks for bringing this back! I was actually thinking about it yesterday during a meeting haha~
Instead of creating an API around it (an experimental option + an environment variable), what do you think of just using an "enable animation" button in the UI + local storage to save the user's animation preference? See previous art:
My thinking is that for a person who doesn't want the effect, I wouldn't want them to have to figure out how to disable it by consulting the documentation and having to modify their code accordingly. Just clicking a button should be enough to end the snow (it also comes with the benefit that if we ever want to remove it for good, we don't have to do any API changes because there are none)
|
Maybe, yes. I spoke with Daniel how to implement it. If he like your idea @lihbr I'll make it! |
|
that's a good point. clicking a button should be enough (as with the animated loading screen from v3) and we could save that to local storage to persist between nuxt dev server sessions. |
|
Aye aye, sir! Add button, |
Hopefully done. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/ui-templates/templates/loading/index.html (1)
87-95: Address the previously flagged issue with method existence check.Line 89 has an incorrect check for
getTimezoneOffsetthat was flagged in a previous review. Please apply the suggested fix to usetypeof y.getTimezoneOffset !== 'function'instead.
🧹 Nitpick comments (1)
packages/ui-templates/templates/loading/index.html (1)
157-158: Improve readability by breaking long lines into multiple statements.These lines perform multiple operations in a single statement, making them difficult to read and maintain.
Apply this diff to improve readability:
- function update() { - $btn.innerHTML = snowEnabled ? '☀️' : '❄️' - $btn.title = snowEnabled ? 'Disable snow' : 'Enable snow' - if (snowEnabled) { resize(); window.addEventListener('resize', resize); $canvas.style.opacity = 1; lastCall = Date.now(); draw() } - else { if (animationId) cancelAnimationFrame(animationId); animationId = null; window.removeEventListener('resize', resize); $canvas.style.opacity = 0 } - } + function update() { + $btn.innerHTML = snowEnabled ? '☀️' : '❄️' + $btn.title = snowEnabled ? 'Disable snow' : 'Enable snow' + if (snowEnabled) { + resize() + window.addEventListener('resize', resize) + $canvas.style.opacity = 1 + lastCall = Date.now() + draw() + } else { + if (animationId) cancelAnimationFrame(animationId) + animationId = null + window.removeEventListener('resize', resize) + $canvas.style.opacity = 0 + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui-templates/templates/loading/index.html(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (2)
packages/ui-templates/templates/loading/index.html (2)
44-48: LGTM! Accessibility guard implemented correctly.The media query properly respects user preferences for reduced motion by hiding the snow canvas.
131-151: LGTM! Animation loop implementation is solid.The particle physics and rendering logic are well implemented with proper delta time handling and canvas wrapping.
| const $btn = document.createElement('button') | ||
| $btn.id = 'snow-toggle' | ||
| $btn.style = 'position:fixed;bottom:16px;right:16px;z-index:100;background:rgba(255,255,255,0.1);border:1px solid rgba(255,255,255,0.2);border-radius:8px;padding:8px 12px;cursor:pointer;font-size:20px;' | ||
| document.body.appendChild($btn) |
There was a problem hiding this comment.
Add accessibility attributes to the toggle button.
The button should include proper accessibility attributes for screen readers.
Apply this diff to add accessibility attributes:
const $btn = document.createElement('button')
$btn.id = 'snow-toggle'
+$btn.setAttribute('aria-label', 'Toggle snow effect')
+$btn.setAttribute('type', 'button')
$btn.style = 'position:fixed;bottom:16px;right:16px;z-index:100;background:rgba(255,255,255,0.1);border:1px solid rgba(255,255,255,0.2);border-radius:8px;padding:8px 12px;cursor:pointer;font-size:20px;'
document.body.appendChild($btn)📝 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.
| const $btn = document.createElement('button') | |
| $btn.id = 'snow-toggle' | |
| $btn.style = 'position:fixed;bottom:16px;right:16px;z-index:100;background:rgba(255,255,255,0.1);border:1px solid rgba(255,255,255,0.2);border-radius:8px;padding:8px 12px;cursor:pointer;font-size:20px;' | |
| document.body.appendChild($btn) | |
| const $btn = document.createElement('button') | |
| $btn.id = 'snow-toggle' | |
| $btn.setAttribute('aria-label', 'Toggle snow effect') | |
| $btn.setAttribute('type', 'button') | |
| $btn.style = 'position:fixed;bottom:16px;right:16px;z-index:100;background:rgba(255,255,255,0.1);border:1px solid rgba(255,255,255,0.2);border-radius:8px;padding:8px 12px;cursor:pointer;font-size:20px;' | |
| document.body.appendChild($btn) |
🤖 Prompt for AI Agents
In packages/ui-templates/templates/loading/index.html around lines 107 to 110,
the toggle button lacks accessibility attributes for screen readers; update the
created button element to include an accessible name and state by adding
attributes such as aria-label="Toggle snow animation" (or a localized label),
title="Toggle snow animation", aria-pressed="false" (toggle to "true" when
active), and aria-controls="<id-of-snow-element>" if it toggles a specific
region; ensure it remains keyboard-focusable (button element already is) and
update aria-pressed programmatically when the button state changes.
There was a problem hiding this comment.
Not sure those are relevant with title below 🤔
There was a problem hiding this comment.
@lihbr You raise a fair point! The title attribute does provide some accessible text. However, there's a subtle but important difference:
titleshows tooltips on hover but isn't consistently announced by all screen readers, especially on mobile devicesaria-labelprovides a reliable accessible name that screen readers will announce
That said, if you're adding aria-label, it should be dynamic (updated in the update() function alongside title) rather than static as suggested. Otherwise, screen reader users would hear the same label regardless of the button's state.
For a toggle button showing only emoji, the most accessible approach would be:
- Update both
aria-labelandtitledynamically inupdate() - Add
aria-pressedattribute to indicate toggle state (true/false)
The type="button" attribute is still good practice to prevent unintended form submissions, even though there's no form context here.
If you prefer to keep it simple and rely on just the dynamic title, that's reasonable for this use case—though it may not provide optimal accessibility for all users.
🧠 Learnings used
Learnt from: Tofandel
Repo: nuxt/nuxt PR: 0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
lihbr
left a comment
lihbr
left a comment
There was a problem hiding this comment.
Looking good to me, thanks for updating it following our discussions :)
|
Thank you @Flo0806 💚 |
5 participants
Summary
It is the season! This PR brings some winter magic to the Nuxt dev loading screen ☃️
prefers-reduced-motionfor accessibilityBecause debugging in December should feel a little more festive 🎄