-
-
Notifications
You must be signed in to change notification settings - Fork 521
Added support for drag/drop of images #282
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
Conversation
Reviewer's GuideThis pull request introduces drag-and-drop file uploads by first refactoring common file processing logic into a new File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @aidanjacobson - I've reviewed your changes - here's some feedback:
- Clarify or adjust
fileTypeand inputacceptattribute logic for scenarios involving multiple, different file types in a single operation or subsequent operations. - Consider separating one-time setup logic (e.g., fetching conversion options, setting initial
fileType) from the per-file processing (e.g., row creation, upload initiation) within thehandleFilefunction. - Consider providing user-facing feedback if no supported files are detected during a drag-and-drop operation, in addition to the current
console.warn.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| row.innerHTML = ` | ||
| <td>${file.name}</td> | ||
| <td><progress max="100"></progress></td> | ||
| <td>${(file.size / 1024).toFixed(2)} kB</td> | ||
| <td><a onclick="deleteRow(this)">Remove</a></td> |
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.
🚨 suggestion (security): Consider sanitizing file name content when inserting into innerHTML.
Encode or sanitize file names before inserting into innerHTML to prevent issues from unexpected characters.
Suggested implementation:
function sanitizeHTML(str) {
var temp = document.createElement('div');
temp.textContent = str;
return temp.innerHTML;
}
// Extracted handleFile function for reusability in drag-and-drop and file input
function handleFile(file) {
const fileList = document.querySelector("#file-list");
const row = document.createElement("tr");
row.innerHTML = `
<td>${sanitizeHTML(file.name)}</td>
<td><progress max="100"></progress></td>
<td>${(file.size / 1024).toFixed(2)} kB</td>
<td><a onclick="deleteRow(this)">Remove</a></td>
`;Ensure that the sanitizeHTML function is placed in an appropriate scope (outside of other functions if needed) so that it can be used throughout the file if similar sanitization is required elsewhere.
| <td>${file.name}</td> | ||
| <td><progress max="100"></progress></td> | ||
| <td>${(file.size / 1024).toFixed(2)} kB</td> | ||
| <td><a onclick="deleteRow(this)">Remove</a></td> |
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.
suggestion: Consider avoiding inline event handlers.
Use JavaScript to attach event listeners instead of inline onclick attributes to improve maintainability and separation of concerns.
Suggested implementation:
row.innerHTML = `
<td>${file.name}</td>
<td><progress max="100"></progress></td>
<td>${(file.size / 1024).toFixed(2)} kB</td>
<td><a class="delete-button" href="#">Remove</a></td>
`; // Attach event listener to the remove button to avoid inline onclick
const deleteButton = row.querySelector(".delete-button");
if (deleteButton) {
deleteButton.addEventListener("click", (event) => {
event.preventDefault();
deleteRow(deleteButton);
});
}Ensure that the deleteRow function is prepared to work with the element passed via the event listener.
|
Looking great! Could you rename the commit to "feat: add support for drag/drop of images" |
|
Yep |
Added support for drag/drop of images
Extracted file handling to handleFile method, then changed both file input and added drag/drop to make use of this function.
Summary by Sourcery
Add drag and drop support for file uploads by extracting common file handling logic into a reusable function
New Features:
Enhancements: