Repository navigation
Conversation
|
Well I guess its the same as https://github.com/wintercms/winter/pull/990/files 😆 |
|
Kind of worked around it using a partial |
|
That looks awesome and will work for 'all' file types. |
|
Hmm or is that only in the mediafinder and not in form widget? Might need to extend it. Need to test it first. |
@AIC-BV It's only related to the mediamanager! ⚠ Your use case is a little more specific ... I think you are using a If it became possible to carry over dynamic icon support to the |
|
@damsfx perhaps we should take the logic for generating the SVG and put it in a helper class so that the mediamanager and the fileupload field can use it? |
|
@LukeTowers Yes, that would be perfect, but I'm not sure how to get started with that change. |
|
@bennothommo do you have any thoughts on a desired API / direction to go in for the proposal of making it easier to reuse the work that was just added for generating icons based on file extension in the media manager? |
|
Sure, we could use the PHP SVG library. Would be very simple to have it generate text on the fly from what I can see. |
|
@LukeTowers @bennothommo Isn't using a library to generate an SVG image (a simple string of characters) a bit over-dimmed? |
|
@LukeTowers Yes, of course. |
|
HTML, maybe here: https://github.com/wintercms/storm/blob/develop/src/Filesystem/Filesystem.php as @bennothommo @jaxwilko @mjauvin any thoughts? |
Move the MediaManager's file extension SVG icon and its styles into a shared backend partial and LESS file, and use them in the FileUpload image modes for files browsers can't display in an <img>, instead of rendering a broken thumbnail. Replaces the earlier PDF iframe approach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe change adds a shared SVG file-icon renderer and file-type styles. The media manager now uses the shared renderer and styles. File-upload responses include an icon for extensions outside the browser-displayable image list. Existing-file and upload previews display the icon when available and otherwise use the thumbnail. The upload styles set icon dimensions for single-image and multi-image previews. Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Default image previews and extension icons follow the intended behavior. Custom extensions outside the fixed displayable set may show icons instead of thumbnails, but the repository does not establish that this is a supported-preview regression. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new icon is escaped before display, and the upload’s existing validation and attachment flow is unchanged. A failure while rendering an icon could nevertheless leave a saved file behind while reporting that the upload failed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
See 1st message in conversation |
LukeTowers
left a comment
There was a problem hiding this comment.
Merge conflicts need to be resolved, the PR description need screenshots / screen recordings so I can review it easier, and is there any way we could make the medialibrary and the fileupload widget use the same base functionality for file type icons?
Moves the CSS variable colours from wintercms#1541 into the shared file-icon.less and recompiles fileupload.css and mediamanager.css. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The FileUpload widget and the MediaManager both render the icon through the helper instead of including the partial by path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review @LukeTowers! Merge conflicts: resolved. They all came from #1541: the Screenshots: there's a before/after table at the bottom of the description (PDF and DOCX in an Shared base for the icons: both widgets already shared the same SVG markup and styles. I've now put that behind a helper, so neither widget points at a partial path anymore:
Each widget still decides when to show the icon. The MediaManager does it by item type. FileUpload uses a fixed list of extensions that browsers can display in an Side note, unrelated to this PR: on current develop, |
Replaces the original iframe approach in this PR with the direction discussed above: reuse the file extension icon from the MediaManager (#1061) in the FileUpload FormWidget.
Problem
In
imagemode, the FileUpload widget always renders<img src="thumbUrl">. For files that aren't images (e.g. a PDF whenfileTypesallows it),getThumb()returns the file path itself, so the widget shows a broken image, both for existing files and right after upload.Changes
Backend::fileIcon(string $extension)on the Backend helper (next toBackend::dateTime()). It renders thebackend::file_iconview (modules/backend/views/file_icon.php), which holds the SVG that used to live inmediamanager/partials/_item-icon.php. The extension is now escaped, since FileUpload file names are user-supplied..file-iconrules (including the CSS variable colours from Update Winter logos & use CSS variables for backend colour #1541) out ofmediamanager.lessintomodules/backend/assets/less/controls/file-icon.less. Bothmediamanager.lessandfileupload.lessimport it, so the icon renders outside the[data-control="media-manager"]scope._item-icon.phpcallsBackend::fileIcon(); the tile view looks the same as before.makeFileIcon($file)helper: returnsBackend::fileIcon()for files browsers can't display in an<img>,nullotherwise._image_single.phpand_image_multi.phprender the icon instead of the<img>for those files.onUpload()returns the icon markup asicon.fileupload.jsswaps it in for the thumbnail on success.Notes
$displayableImageExtensions: avif, bmp, gif, ico, jpeg, jpg, png, svg, webp), notFileDefinitions::get('imageExtensions'). That definition is configurable viacms.fileDefinitions, and some installs widen it to allow e.g. PDFs in image fields, which is exactly the case this PR fixes.fileupload.cssandmediamanager.csswere recompiled.Testing
Tested in a Winter 1.2 backend with an
attachManyfield (image-multi) and anattachOnefield (image-single):🤖 Generated with Claude Code