-
-
Notifications
You must be signed in to change notification settings - Fork 237
feat: generate sprite for file view #1347
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
feat: generate sprite for file view #1347
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds an SVG sprite-based icon system for the file tree. Vue components (DirectoryListing.vue, FileTree.vue) now render inline SVG references to a generated Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/Code/DirectoryListing.vue (1)
4-4:⚠️ Potential issue | 🔴 CriticalMissing import for
ADDITIONAL_ICONS.The template references
ADDITIONAL_ICONS['folder']on lines 90 and 117, but onlygetFileIconis imported. This will cause a runtime reference error.🐛 Proposed fix to add missing import
-import { getFileIcon } from '~/utils/file-icons' +import { ADDITIONAL_ICONS, getFileIcon } from '~/utils/file-icons'app/components/Code/FileTree.vue (1)
4-4:⚠️ Potential issue | 🔴 CriticalMissing import for
ADDITIONAL_ICONS.The template references
ADDITIONAL_ICONS['folder-open']andADDITIONAL_ICONS['folder']on line 68, but onlygetFileIconis imported. This will cause a runtime reference error.🐛 Proposed fix to add missing import
-import { getFileIcon } from '~/utils/file-icons' +import { ADDITIONAL_ICONS, getFileIcon } from '~/utils/file-icons'
🧹 Nitpick comments (1)
scripts/generate-file-tree-sprite.ts (1)
75-81: Consider deduplicating icon names to avoid redundant symbols.Multiple icon mappings may reference the same icon (e.g., different extensions mapping to the same icon). This could result in duplicate
<symbol>entries in the sprite. Using aSetwould ensure each icon is only included once.♻️ Proposed improvement to deduplicate icons
const iconNames = [ ...Object.values(EXTENSION_ICONS), ...Object.values(FILENAME_ICONS), ...Object.values(COMPOUND_EXTENSIONS), ...Object.values(ADDITIONAL_ICONS), DEFAULT_ICON, ] - const grouped = groupByCollection(iconNames) + const uniqueIconNames = [...new Set(iconNames)] + const grouped = groupByCollection(uniqueIconNames)
Optimization of css bundle (53.6kb vs 150kb)
Moved icons of file-viewer to the generated sprite and included them via
svg+useThe sprite is 91kb, but it doesn't load until icons with a link to this sprite start appearing (i.e. we will load it only on code pages)
It's generated by a script based on the same file with icons sets as used in file-viewer (I just changed the names slightly so Uno wouldn't pick it up) on
postinstallRelated #1239 (or closes?)