Skip to content
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.

Sign up for GitHub

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

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

Merged
merged 4 commits into from
Jan 11, 2021
Merged

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0caafc
add new image upload system tests
noi5e Jan 11, 2021
34340bd
rename comment fixtures
noi5e Jan 11, 2021
a83d08f
assign D.selected in start; rewrite drop assignment #8926
noi5e Jan 11, 2021
ca17ce3
specify E.refresh conditional call in E.wrap #8926
noi5e Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions app/assets/javascripts/dragdrop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,27 @@ jQuery(function() {
});
$('.dropzone').on('drop',function(e) {
// this 'drop' listener is also reused for pages with just one form, ie. /wiki/new
$D.selected = $(e.target).closest('div.comment-form-wrapper').eq(0);
const closestCommentFormWrapper = e.target.closest('div.comment-form-wrapper'); // this returns null if there is no match
e.preventDefault();
let params = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote this part for greater readability, using JavaScript's closest (as opposed to jQuery's method with the same name).

JavaScript's closest returns null, whereas jQuery's returns an object. I thought this would be more intuitive.

// $D.selected will look different for multi vs. single form pages, because of what $.closest returns:
// multiple comments: { 0: the_closest_element }
// /wiki/new: .comment-form-wrapper doesn't exist!
if ($D.selected.hasOwnProperty(0)) { // eg. jQuery finds a .comment-form-wrapper somewhere on the page.
params['textarea'] = $D.selected[0].querySelector('textarea').id // assign the ID of the textarea within the closest comment-form-wrapper
// there are no .comment-form-wrappers on /wiki/edit or /wiki/new
// these pages just have a single text-input form.
if (closestCommentFormWrapper) {
$D.selected = $(closestCommentFormWrapper);
// assign the ID of the textarea within the closest comment-form-wrapper
params['textarea'] = closestCommentFormWrapper.querySelector('textarea').id;
} else {
// default to #text-input
// ideally there should only be one per page
params['textarea'] = 'text-input'
params['textarea'] = 'text-input';
}
$E.initialize(params);
});
$('#side-dropzone').on('drop',function(e) {
e.preventDefault();
});

// these functions are also used on /wiki/edit and /wiki/new pages
$('.dropzone').each(function() {
$(this).fileupload({
url: "/images",
Expand All @@ -63,6 +65,13 @@ jQuery(function() {
'nid':$D.nid
},
start: function(e) {
// 'start' function runs:
// 1. when user drag-and-drops image
// 2. when user clicks on upload button.
// for click-upload-button scenarios, it's important to set $D.selected here, because the 'drop' listener above doesn't run in those:
$D.selected = $(e.target).closest('div.comment-form-wrapper');
// the above line is redundant in drag & drop, because it's assigned in 'drop' listener too.
// on /wiki/new & /wiki/edit, $D.selected will = undefined from this assignment
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a major change: assign $D.selected in the start upload function.

As a reminder, this function runs on both click and drop image upload events. It's important to assign $D.selected here for click events, because the drop listener doesn't run for those.

I left a comment in the code about this, but I'm thinking it would be good down the road to not have to assign $D.selected twice in a row for drop events. Not sure how to do that yet.

$('#imagebar .inline_image_drop').show();
$('#create_progress').show();
$('#create_uploading').show();
Expand Down
32 changes: 17 additions & 15 deletions app/assets/javascripts/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
$E = {
initialize: function(args) {
args = args || {}
// title
$E.title = $('#title')
// textarea
args['textarea'] = args['textarea'] || 'text-input'
$E.textarea = $('#'+args['textarea'])
$E.title = $('#title')
$E.textarea.bind('input propertychange',$E.save)
// preview
args['preview'] = args['preview'] || 'preview'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additions here, just rearranged stuff for greater readability. Sometime soon, I'd like to rewrite this part to use JavaScript ES6's default parameters: initialize(textarea = 'text-input')

$E.preview = $('#'+args['preview'])
$E.textarea.bind('input propertychange',$E.save)


marked.setOptions({
gfm: true,
tables: true,
Expand All @@ -23,24 +26,25 @@ $E = {
return code;
}
});

},
is_editing: function() {
return ($E.textarea[0].selectionStart == 0 && $E.textarea[0].selectionEnd == 0)
},

refresh: function() {
if($D.selected) {
$E.textarea = ($D.selected).find('textarea').eq(0);
$E.preview = ($D.selected).find('#preview').eq(0);
$E.textarea.bind('input propertychange',$E.save);
}
// textarea
$E.textarea = ($D.selected).find('textarea').eq(0);
$E.textarea.bind('input propertychange',$E.save);
// preview
$E.preview = ($D.selected).find('#preview').eq(0);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think these lines needed to be inside of a conditional checking for $D.selected (it made it little less readable). I moved the conditional into $E.wrap


// wraps currently selected text in textarea with strings a and b
wrap: function(a,b,args) {
var isWiki = (window.location + '').includes('wiki');
if (!isWiki) this.refresh();
// this RegEx is: /wiki/ + any char not "/" + /edit
const isWikiCommentPage = (/\/wiki\/[^\/]+\/edit/).test(window.location.pathname);
// we don't need to refresh $E's values if we're on a page with a single comment form, ie. /wiki/new or /wiki/edit
if (window.location.pathname !== "/wiki/new" && !isWikiCommentPage && $D.selected) {
this.refresh();
}
var len = $E.textarea.val().length;
Copy link
Contributor Author

@noi5e noi5e Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty big change too. The (window.location + '').includes('wiki') part was originally done in #5650 to fix image upload in /wiki/new and /wiki/{wiki name}/edit...

Unfortunately this had the side effect of running refresh on wiki comments pages (which include('wiki') in the window.location). I finally figured out this doesn't need to happen, because the wiki#show view just loads comments through note#comments. This used to make writing system tests more difficult because things ran differently for wiki comments.

The last note is that I'm thinking that this.refresh(); may not need to be called at all. We could remove it entirely if we assign $D.selected in all the right places, and if $E.textarea remains constant on /wiki/new and /wiki/edit. But that's for another PR.

var start = $E.textarea[0].selectionStart;
var end = $E.textarea[0].selectionEnd;
Expand Down Expand Up @@ -68,7 +72,6 @@ $E = {
image: function(src) {
$E.wrap('\n![',']('+src+')\n')
},

h1: function() {
$E.wrap('#','')
},
Expand Down Expand Up @@ -123,7 +126,6 @@ $E = {
toggle_preview: function (comment_id = null) {
let previewBtn;
let dropzone;

// if the element is part of a multi-comment page,
// ensure to grab the current element and not the other comment element.
if (comment_id) {
Expand Down
6 changes: 3 additions & 3 deletions test/fixtures/comments.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,21 +218,21 @@ ouija_comment_3:
comment: O
timestamp: <%= Time.now.to_i + 30 %>

wiki_comment_1: # wiki fixture for testing multiple comments
pokemon_comment_1: # wiki fixture for testing multiple comments
uid: 1
nid: 42
status: 1
comment: Squirtle
timestamp: <%= Time.now.to_i + 10 %>

wiki_comment_2:
pokemon_comment_2:
uid: 5
nid: 42
status: 1
comment: Charmander
timestamp: <%= Time.now.to_i + 20 %>

wiki_comment_3:
pokemon_comment_3:
uid: 6
nid: 42
status: 1
Expand Down
Loading