add drop-to-replace functionality for image prompt drop zone#1341
add drop-to-replace functionality for image prompt drop zone#1341GlenCarpenter wants to merge 5 commits intomcmonkeyprojects:masterfrom
Conversation
|
this is related to #1269 |
| let type = img.src.substring(img.src.lastIndexOf('.') + 1); | ||
| let file = new File([blob], imagePathClean, { type: `image/${type.length > 0 && type.length < 20 ? type : 'png'}` }); | ||
| imagePromptAddImage(file); | ||
| imagePromptAddOrReplaceImage(file); |
There was a problem hiding this comment.
this function rename makes the commit messy, keep the original function name
There was a problem hiding this comment.
I originally renamed the old version of imagePromptAddImage to imagePromptAddImageCore and called it in a new imagePromptAddImage, but this feels more descriptive to me. "adding" and "replacing" are two separate actions and this is a function that figures out which action to take. That being said it's not a hill I am going to die on so I can change it back if you feel it makes more sense.
There was a problem hiding this comment.
got rid of the unnecessary functions
src/wwwroot/js/genpage/main.js
Outdated
| } | ||
| } | ||
|
|
||
| function isImageFileDragEvent(e) { |
There was a problem hiding this comment.
this function is... odd. There's existing handling for drag/drop stuff
There was a problem hiding this comment.
I will move this to the other event handling logic.
src/wwwroot/js/genpage/main.js
Outdated
| } | ||
|
|
||
| function getPromptImageDropReplaceTarget(e) { | ||
| if (!isImageFileDragEvent(e) || !e.target || !e.target.closest) { |
There was a problem hiding this comment.
why would element.closest ever not exist
There was a problem hiding this comment.
old habit I guess. It has full browser support now so this check isn't needed.
| return; | ||
| } | ||
| let reader = new FileReader(); | ||
| reader.onload = (e) => { |
There was a problem hiding this comment.
there should not be new file reading code, there already is code for this stuff
There was a problem hiding this comment.
Will refactor to use the existing file reader code.
There was a problem hiding this comment.
consolidated a bunch of code
src/wwwroot/css/genpage.css
Outdated
| cursor: pointer; | ||
| } | ||
| .alt-prompt-image-container.image-drop-replace-target .alt-prompt-image { | ||
| outline: 2px solid white; |
There was a problem hiding this comment.
@mcmonkey4eva I am going to update this to match the :hover state for thumbnails
outline: 2px solid var(--box-selected-border-stronger);
border-radius: 5px;
There was a problem hiding this comment.
ended up with
.alt-prompt-image-container.image-drop-replace-target .alt-prompt-image {
outline: 4px solid var(--emphasis);
border-radius: 5px;
}
This makes it more obvious the image is being targeted.
src/wwwroot/js/util.js
Outdated
| } | ||
|
|
||
| /** Reads the given file as a data URL and passes the result to the given handler. Ignores null file inputs. */ | ||
| function readFileAsDataURL(file, handler) { |
There was a problem hiding this comment.
where did this come from now? again, there's already code that handles this part, you have no reason to be committing new file reading
There was a problem hiding this comment.
I thought this was the existing code you were referencing. What is the point of this module if we are going to create inline FileReader?
There was a problem hiding this comment.
The key rule I think you're missing here is: a PR should always seek to create the minimum possible changes needed to achieve its stated goal. Refactoring and reorganizing should not happen in a PR unless that is the focused purpose of a PR, which this is not.
There's already code that handles reading a file in the correct place, so you should not be modifying it at all here, just redirecting where the resultant image goes.
There was a problem hiding this comment.
Makes sense, I see what you're saying. I'll restore the old FileReader and keep all the new code localized to the existing imagePromptAddImage
|
don't do the forcepushes btw, just do regular commits, it'll be squashed on final pull |
src/wwwroot/js/genpage/main.js
Outdated
| } | ||
|
|
||
| function getPromptImageDropReplaceTarget(e) { | ||
| if (!e || !e.dataTransfer || !e.target || !e.target.closest || uiImprover.getFileList(e.dataTransfer, e).length == 0) { |
There was a problem hiding this comment.
again these are strange checks that don't make sense
There was a problem hiding this comment.
Do you mean just the e.target.closest or all of the event checks?
There was a problem hiding this comment.
The closest one is the obvious problem, but I imagine most of the checks here can never fail, and so don't need to be checked
2 participants
add drop-to-replace functionality for image prompt drop zone
a2pxwhiteoutline: 4px solid var(--emphasis)to indicate file will be replaced2pxmargin to images in drop zone to prevent outline overlap