-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Show Image Upload Progress Bars in Edit Comment CLICK-to-Upload #9019
Show Image Upload Progress Bars in Edit Comment CLICK-to-Upload #9019
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9019 +/- ##
=======================================
Coverage ? 82.03%
=======================================
Files ? 100
Lines ? 5933
Branches ? 0
=======================================
Hits ? 4867
Misses ? 1066
Partials ? 0 |
@@ -17,44 +17,36 @@ jQuery(function() { | |||
); | |||
} | |||
|
|||
$('.dropzone').on('dragover',function(e) { | |||
e.preventDefault(); |
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.
I stuck these listeners in the $('.dropzone').each
further down. Having them inside that function lets me use $(this)
to attach an eventListener... Which reduces the number of times that the same eventListener is attached to the same element.
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.
Amazing 👍
e.preventDefault(); | ||
$(e.currentTarget).addClass('hover'); | ||
}); | ||
|
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.
Changed dragover
and dragout
to dragenter
and dragleave
so they don't fire every time the user makes a movement with the image on the surface of the div.
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.
wow good thinking 🎉
font-size: inherit; | ||
color: inherit; | ||
white-space: pre-wrap; | ||
background-color: transparent; | ||
border-radius: 0; | ||
} | ||
} | ||
|
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.
I believe this was an extra close curly-brace that may have been affecting the CSS! Also fixed the commenting before this block.
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.
Excellent!
@@ -118,7 +118,6 @@ table th a:hover { | |||
visibility: hidden; | |||
} | |||
|
|||
.question-show textarea, | |||
.question-show pre { | |||
margin: 0; |
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.
I removed this CSS reference to fix a styling issue in Edit Comment forms. I believe this was done originally to fix #1304, which I think was for the publiclab.org/questions
route, so unrelated to comments on questions.
This is what the Edit Comment looked like before this change (no border color, no padding):
jQuery(document).ready(function() { | ||
$E.initialize(); | ||
}); | ||
|
||
function setInit(id) { |
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.
I removed this $E.initialize
call, I think it's unnecessary because it's already done in so many other places on a page with comments.
@jywarren Okay, ready to merge! |
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.
This is awesome 🎉
I think a better approach to find the divs
or anything inside a comment editor would be the direct use of id
's, like instead of trying to find the closest DOM element inside form like:
e.target.closest('div.comment-form-wrapper');
It would be better to access it directly like:
comment-form-wrapper-2131
where 2131
is some unique id.
Advantage being we are sure that even if their are multiple forms, we are always manipulating the correct DOM elements.
Saying that, the current approach is also fine for now 😄
@@ -17,44 +17,36 @@ jQuery(function() { | |||
); | |||
} | |||
|
|||
$('.dropzone').on('dragover',function(e) { | |||
e.preventDefault(); |
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.
Amazing 👍
e.preventDefault(); | ||
$(e.currentTarget).addClass('hover'); | ||
}); | ||
|
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.
wow good thinking 🎉
@@ -115,15 +102,27 @@ jQuery(function() { | |||
}, | |||
|
|||
// see callbacks at https://github.com/blueimp/jQuery-File-Upload/wiki/Options | |||
fileuploadfail: function(e,data) { | |||
// fileuploadfail: function(e,data) { |
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.
Should we show a toast/noty with "something went wrong while uploading, kindly report an issue if this persists"?
@jywarren thoughts?
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.
Or atleast printing in console the e
error msg for quick debugging?
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.
That's a good idea, can do it in a follow-up PR
font-size: inherit; | ||
color: inherit; | ||
white-space: pre-wrap; | ||
background-color: transparent; | ||
border-radius: 0; | ||
} | ||
} | ||
|
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.
Excellent!
Code Climate has analyzed commit 162fce6 and detected 0 issues on this pull request. View more on Code Climate. |
@Sagarpreet Thanks for the detailed review! I just resolved the merge conflict, and tests are passing, so this one's ready to merge again. @jywarren @cesswairimu I totally agree about referring to comment forms with a unique ID rather than |
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.
This looks amazing, i appreciate all the small and careful tweaks, fixes, and documentation. it was easy to read and review and @Sagarpreet's review is also super helpful, thanks!
Approving and merging!!!
…ubliclab#9019) * add tests for image upload progress bars publiclab#9018 * fix image upload hover & progress bar issues publiclab#9018 * change ID references to class references publiclab#9018 * fix style for edit comment forms on questions publiclab#9018 * add classes to image upload progress bars publiclab#9018 * add classes; remove non-unique IDs publiclab#9018 * tweak image upload progress bar tests publiclab#9018 * forgot to remove screenshot publiclab#9018
…ubliclab#9019) * add tests for image upload progress bars publiclab#9018 * fix image upload hover & progress bar issues publiclab#9018 * change ID references to class references publiclab#9018 * fix style for edit comment forms on questions publiclab#9018 * add classes to image upload progress bars publiclab#9018 * add classes; remove non-unique IDs publiclab#9018 * tweak image upload progress bar tests publiclab#9018 * forgot to remove screenshot publiclab#9018
…ubliclab#9019) * add tests for image upload progress bars publiclab#9018 * fix image upload hover & progress bar issues publiclab#9018 * change ID references to class references publiclab#9018 * fix style for edit comment forms on questions publiclab#9018 * add classes to image upload progress bars publiclab#9018 * add classes; remove non-unique IDs publiclab#9018 * tweak image upload progress bar tests publiclab#9018 * forgot to remove screenshot publiclab#9018
…ubliclab#9019) * add tests for image upload progress bars publiclab#9018 * fix image upload hover & progress bar issues publiclab#9018 * change ID references to class references publiclab#9018 * fix style for edit comment forms on questions publiclab#9018 * add classes to image upload progress bars publiclab#9018 * add classes; remove non-unique IDs publiclab#9018 * tweak image upload progress bar tests publiclab#9018 * forgot to remove screenshot publiclab#9018
Fixes #9018 (see this issue page for a more detailed write-up & game plan)
This fixes the original bug AND I made a few extra changes to image upload CSS:
.progress-bar
's CSS width to 0 after a fileupload, so it can be reused for a second upload.hover
class was not appearing (the div wasn't darkening) when user hovered an image over the.dropzone
.Before Fixes
After Fixes
(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)