Integrate Edit Comment Form JavaScript with editor.js #9067

noi5e · 2021-01-23T18:25:49Z

Part of an object-oriented refactoring of editor.js. See #9004 for more details.

Please merge #9062 first!

SUMMARY:
In the past, there were two separate kinds of comment form partials:

REPLY and MAIN comment forms rendered at /app/views/comments/_form.html.erb
EDIT comment forms rendered at /app/views/comments/_edit.html.erb

To boot, these two different comment form types relied on two separate sources for JavaScript editor functions like image upload, and rich-text editing:

REPLY and MAIN comment forms rely on editor.js.
EDIT comment forms rely on a <script> tag within the partial here

This made code maintainability more difficult because if a change was made in editor.js, it had to account for the <script> tag.

This PR integrates the two JS sources. Now _edit.html.erb relies on editor.js! Hooray!

CHANGES:

I changed CSS IDs in _edit.html.erb so that editor.js can access them:

#c123preview is now #comment-preview-edit-123
#c123text is now #text-input-edit-123

Deleted <script> tag in _edit.html.erb so now it relies on editor.js instead.

Slightly unrelated changes:

I deleted setInit function definition and calls in _edit.html.erb. setInit was kind of like a way that we used to setState in EDIT comment forms before. The problem is that we called it whenever the user opened up an edit comment form, or clicked on Preview, which I don't think is a good place to setState. See this comment for an explanation.
Add a .dropzone class to #c123div, mainly so editor.js can toggle preview (Yes, we toggle on .dropzone when the user clicks on "Preview")
Move $E.initialize() call from _form.html.erb (REPLY and MAIN comment form partial) into higher level partials. This is so that editor.js isn't initialized 5 times on a page with 5 comments (this was happening before! 😱)

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

gitpod-io · 2021-01-23T18:25:53Z

codecov · 2021-01-23T18:31:59Z

Codecov Report

❗ No coverage uploaded for pull request base (main@7b4bc4d). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #9067   +/-   ##
=======================================
  Coverage        ?   82.15%           
=======================================
  Files           ?      100           
  Lines           ?     5936           
  Branches        ?        0           
=======================================
  Hits            ?     4877           
  Misses          ?     1059           
  Partials        ?        0

noi5e · 2021-01-23T18:52:01Z

app/views/notes/_comment.html.erb

@@ -53,7 +53,7 @@
        <i data-toggle="tooltip" title="This comment was posted by email." class="fa fa-envelope"></i>
      <% end %>
      <% if current_user && comment.uid == current_user.uid %>
-        <a aria-label="Edit comment" class="btn btn-outline-secondary btn-sm" id="edit-comment-btn" href="javascript:void(0)" onClick="$('#c<%= comment.cid %>edit').toggle();$('#c<%= comment.cid %>show').toggle();setInit(<%= comment.cid %>);">
+        <a aria-label="Edit comment" class="btn btn-outline-secondary btn-sm edit-comment-btn" href="javascript:void(0)" onClick="$('#c<%= comment.cid %>edit').toggle();$('#c<%= comment.cid %>show').toggle();">


Small, unrelated change: I changed this to a class instead of an ID.

noi5e · 2021-01-23T18:53:02Z

app/views/comments/_edit.html.erb

+              $E.setState(
+                'text-input-edit-<%= comment.id%>', 
+                'comment-preview-edit-<%= comment.id %>'
+              );


Because editor.js has a new setState method, I can call it here instead of the old inline JavaScript.

noi5e · 2021-01-23T18:53:51Z

app/views/comments/_edit.html.erb

-        <span id="c<%= comment.id%>prompt" class="prompt choose-one-prompt-text">
-          <span style="padding-right:4px;float:left;" class="hidden-xs">
-          <%= raw translation('comments._edit.drag_and_drop') %>
+    <div id="c<%= comment.id%>div" class="form-group dropzone">


Add the class .dropzone here because this is what editor.js uses to toggle preview.

This seems a little odd sounding - isn't dropzone supposed to mean the area we can drag a file/image into? Coudl this be an artifact from when preview and drag-drop were implemented at the same time, and might we think of a more legible classname or of separating preview and drag-drop?

It kind of makes sense that we remove .dragdrop when we turn on preview, maybe... so you can't dragdrop onto the preview... but I don't know, shouldn't it just hide the dragdrop and not remove the class? This suggests to me that we should separate the functions and namings to make it more legible and less interdependent, whether or not I'm really understanding what's happening in the current model. What do you think?

Thanks, @noi5e !!!

@jywarren I am totally in agreement that .dropzone is not really the best class to refer to in this instance. I think the reason this was done in the past has to do with this:

What we're toggling on is B in the above image. I think in the past, .dropzone was toggled for preview because it was just a way of referring to "The visible, largest part of the comment form". I acknowledge it's weird, but I was just following the precedent of older code.

For example, it's happening here in this legacy code in editor.js:

plots2/app/assets/javascripts/editor.js

Lines 141 to 146 in b300979

previewBtn = $(this.textarea.context).find('.preview-btn');

dropzone = $(this.textarea.context).find('.dropzone');

$E.preview[0].innerHTML = marked($E.textarea.val());

$E.preview.toggle();

dropzone.toggle();

I think the best thing to do moving forward is to add another CSS ID/class 😢 that has the same coverage as .dropzone in this instance, and toggle preview on that. Adding it to my planning issue so I don't forget to do this.

Oh, thanks for the explanation! This makes total sense and is the kind of thing that is hard to plan for in the moment, but is really helpful down the road later. A second class sounds like just the right solution!

noi5e · 2021-01-26T17:50:16Z

app/views/comments/_edit.html.erb

-            if (data.result['filename'].substr(-3,3) == "JPG") is_image = true
-            if (data.result['filename'].substr(-4,4) == "JPEG") is_image = true
-            if (data.result['filename'].substr(-3,3) == "PNG") is_image = true
-            if (data.result['filename'].substr(-3,3) == "GIF") is_image = true


I kind of just deleted this code, but can definitely save some of it by either:

placing it back in the partial, but commenting it out

saving choice lines and moving it to editorToolbar.js (which used to be dragdrop.js)

Like, here's a similar thing happening in editorToolbar.js:

plots2/app/assets/javascripts/editorToolbar.js

Lines 103 to 107 in d2e249d

var extension = data.result['filename'].split('.')[data.result['filename'].split('.').length - 1]; var file_url = data.result.url.split('?')[0]; var file_type;

if (['gif', 'GIF', 'jpeg', 'JPEG', 'jpg', 'JPG', 'png', 'PNG'].includes(extension))

file_type = 'image'

else if (['csv', 'CSV'].includes(extension))

file_type = 'csv'

Which JS is preferable in this instance?

if (['gif', 'GIF', 'jpeg', 'JPEG', 'jpg', 'JPG', 'png', 'PNG'].includes(extension))

this seems like a MUCH more compact way to do this. And, editorToolbar.js has the CSV detection too! Nice! Let's just rely on that more refined code! 👍

Good to know! Finished resolving merge conflicts about an hour ago. I restored the code and left it commented out.

noi5e · 2021-01-26T17:51:15Z

app/views/comments/_edit.html.erb

-            if (is_image) {
-              image_url = data.result.url.split('?')[0];
-              orig_image_url = image_url.replace('medium','original');
-              $E.wrap('[![',']('+image_url+')]('+orig_image_url+')', {'newline': true, 'fallback': data.result['filename']});


For example Jeffrey you were saying in our meeting today that these lines that are resizing the image display to be smaller might be useful to keep.

Looking at editorToolbar.js i think this already exists there so we're all good!

plots2/app/assets/javascripts/editorToolbar.js

Lines 108 to 112 in d2e249d

switch (file_type) {

case 'image':

orig_image_url = file_url + '?s=o' // size = original

$E.wrap('[![', '](' + file_url + ')](' + orig_image_url + ')', {'newline': true, 'fallback': data.result['filename']}) // on its own line; see /app/assets/js/editor.js

break;

jywarren · 2021-01-26T21:36:20Z

Looks like a system test failure -- comment system tests -- actually really reassuring that these are getting triggered!!!


ERROR CommentTest#test_note:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form (433.92s)
Minitest::UnexpectedError:         NoMethodError: undefined method `[]' for nil:NilClass
            test/system/comment_test.rb:344:in `block (2 levels) in <class:CommentTest>'

==============[Screenshot]: tmp/screenshots/failures_test_question:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form.png
ERROR CommentTest#test_question:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form (687.53s)
Minitest::UnexpectedError:         NoMethodError: undefined method `[]' for nil:NilClass
            test/system/comment_test.rb:344:in `block (2 levels) in <class:CommentTest>'

====[Screenshot]: tmp/screenshots/failures_test_wiki:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form.png
ERROR CommentTest#test_wiki:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form (833.77s)
Minitest::UnexpectedError:         NoMethodError: undefined method `[]' for nil:NilClass
            test/system/comment_test.rb:344:in `block (2 levels) in <class:CommentTest>'

codeclimate · 2021-01-26T21:48:35Z

Code Climate has analyzed commit ed09de9 and detected 0 issues on this pull request.

View more on Code Climate.

noi5e · 2021-01-26T21:50:53Z

Looks like a system test failure -- comment system tests -- actually really reassuring that these are getting triggered!!!

Yep! My first PR got merged into main this morning and created some merge conflicts here. I just resolved some of them, but have to update some selectors in the system tests. This was caused because #c123preview had become #comment-preview-edit-123. There might be a few more instances of this; doing my best to update everything.

noi5e · 2021-01-26T22:06:10Z

@jywarren Tests passing again, ready to merge!

* move E.initialize call to higher level views publiclab#9004 * change selectors for preview & textarea elements publiclab#9004 * change #edit-comment-btn to .edit-comment-btn publiclab#9004 * change selectors to match new edit form publiclab#9004 * update selectors for edit form publiclab#9004

noi5e added 4 commits January 23, 2021 10:17

move E.initialize call to higher level views publiclab#9004

86db66d

change selectors for preview & textarea elements publiclab#9004

30dc90d

change #edit-comment-btn to .edit-comment-btn publiclab#9004

69c544a

change selectors to match new edit form publiclab#9004

4abe44f

noi5e requested a review from a team as a code owner January 23, 2021 18:25

noi5e added feature explains that the issue is to add a new feature JavaScript outreachy labels Jan 23, 2021

noi5e added this to the [Outreachy] Comment Editor Winter 2020 milestone Jan 23, 2021

noi5e added the readytomerge label Jan 23, 2021

noi5e changed the title ~~Edit form js integration~~ Integrate _edit.html.erb JavaScript with editor.js Jan 23, 2021

noi5e changed the title ~~Integrate _edit.html.erb JavaScript with editor.js~~ Integrate Edit Comment Form JavaScript with editor.js Jan 23, 2021

noi5e requested review from sagarpreet-chadha, cesswairimu and jywarren January 23, 2021 18:50

noi5e commented Jan 23, 2021

View reviewed changes

This was referenced Jan 23, 2021

OLD CLOSED Comment Editor Overhaul Issue #8775

Closed

Comment Editor Overhaul Project [Outreachy] #9069

Open

noi5e commented Jan 26, 2021

View reviewed changes

Merge branch 'main' into edit-form-JS-integration

90282b2

update selectors for edit form publiclab#9004

ed09de9

jywarren approved these changes Jan 26, 2021

View reviewed changes

jywarren merged commit 2590128 into publiclab:main Jan 26, 2021

noi5e deleted the edit-form-JS-integration branch January 26, 2021 22:12

noi5e mentioned this pull request Feb 3, 2021

Change #dropzone-large ID to #comment-form-body #9123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Edit Comment Form JavaScript with editor.js #9067

Integrate Edit Comment Form JavaScript with editor.js #9067

noi5e commented Jan 23, 2021 •

edited

gitpod-io bot commented Jan 23, 2021 •

edited

codecov bot commented Jan 23, 2021 •

edited

noi5e Jan 23, 2021

noi5e Jan 23, 2021

noi5e Jan 23, 2021

jywarren Jan 26, 2021

noi5e Jan 27, 2021

jywarren Jan 27, 2021

noi5e Jan 26, 2021

jywarren Jan 26, 2021

noi5e Jan 26, 2021

noi5e Jan 26, 2021

jywarren Jan 26, 2021

jywarren commented Jan 26, 2021

codeclimate bot commented Jan 26, 2021

noi5e commented Jan 26, 2021

noi5e commented Jan 26, 2021

	previewBtn = $(this.textarea.context).find('.preview-btn');
	dropzone = $(this.textarea.context).find('.dropzone');

	$E.preview[0].innerHTML = marked($E.textarea.val());
	$E.preview.toggle();
	dropzone.toggle();

	var extension = data.result['filename'].split('.')[data.result['filename'].split('.').length - 1]; var file_url = data.result.url.split('?')[0]; var file_type;
	if (['gif', 'GIF', 'jpeg', 'JPEG', 'jpg', 'JPG', 'png', 'PNG'].includes(extension))
	file_type = 'image'
	else if (['csv', 'CSV'].includes(extension))
	file_type = 'csv'

	switch (file_type) {
	case 'image':
	orig_image_url = file_url + '?s=o' // size = original
	$E.wrap('[![', '](' + file_url + ')](' + orig_image_url + ')', {'newline': true, 'fallback': data.result['filename']}) // on its own line; see /app/assets/js/editor.js
	break;

Integrate Edit Comment Form JavaScript with editor.js #9067

Integrate Edit Comment Form JavaScript with editor.js #9067

Conversation

noi5e commented Jan 23, 2021 • edited

gitpod-io bot commented Jan 23, 2021 • edited

codecov bot commented Jan 23, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jan 26, 2021

codeclimate bot commented Jan 26, 2021

noi5e commented Jan 26, 2021

noi5e commented Jan 26, 2021

noi5e commented Jan 23, 2021 •

edited

gitpod-io bot commented Jan 23, 2021 •

edited

codecov bot commented Jan 23, 2021 •

edited