Refactor Editor Constructor and Methods, Write Editor Getters #9110

noi5e · 2021-02-02T00:55:20Z

Related to #9004, OOP refactor of Comment/Legacy Editor.

Wrote a new constructor with an added parameter for element ID prefixes.
- I did this to allow for different naming conventions in the legacy wiki editor vs. multi-comment pages.
- On multi-comment pages, the main preview element is named #comment-preview-main
- On the legacy wiki editor, the single preview element should be #preview-main, not also #comment-preview-main
- Hence, this change! You can now $E = new Editor("main", { "preview": "preview" }) to customize element IDs.
- See this comment for more context.
Changed IDs and Editor instantiation accordingly on the legacy wiki editor.
- I also noticed that the image upload progress bars were buggy on wiki pages, so did some CSS tweaks and added new IDs to make them work again.
Also in editor.js, I wrote getter methods to retrieve $E.textarea, $E.textAreaValue, and $E.preview.
Finally also in editor.js, changed most instances of $E.method() to this.method().

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

…his.method publiclab#9004

gitpod-io · 2021-02-02T00:55:24Z

codecov · 2021-02-02T01:01:15Z

Codecov Report

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

@@           Coverage Diff           @@
##             main    #9110   +/-   ##
=======================================
  Coverage        ?   50.04%           
=======================================
  Files           ?      100           
  Lines           ?     6112           
  Branches        ?        0           
=======================================
  Hits            ?     3059           
  Misses          ?     3053           
  Partials        ?        0

noi5e · 2021-02-02T01:07:08Z

app/assets/javascripts/editor.js

-    $E.title = $('#' + title + 'title'); // not sure why this exists? seems like $E.title is always #title
-    $E.textarea = $('#' + textarea);
+  }
+  setState(commentFormID) {


Removing the default parameter (commentFormID = "main") here. I think it makes sense so that errors will be easier to catch.

noi5e · 2021-02-02T01:07:56Z

app/assets/javascripts/editor.js

+  }
+  setState(commentFormID) {
+    this.commentFormID = commentFormID;
+    this.textarea = $("#text-input-" + commentFormID);
    $E.textarea.bind('input propertychange', $E.save);


I changed most instances of $E.method to this.method except for this one here. I toyed around with this, but it opens up a can of worms and is out of scope of this PR. Will go back to this when I refactor save and recover functions.

Just to explain this a little further, it's because when $E.textarea is changed to this.textarea it brings up JavaScript this weirdness. The eventListener for input propertychange (which I'm not sure is a valid event type? I tried Googling it) gets attached to the wrong this.

noi5e · 2021-02-02T01:08:25Z

app/assets/javascripts/editor.js

+  get textAreaElement() {
+    const textAreaID = "#text-input-" + this.commentFormID;
+    return $(textAreaID);
+  }


New getter functions!

Hmm, are these transpiled out in the asset pipeline? Any worries about internet explorer? I guess not these days!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

I mean, i guess it's still 5% of usage: https://www.netmarketshare.com/browser-market-share.aspx?qprid=2&qpcustomd=0

what do you think?

Oh huh... That's a good point. I hadn't checked this one out in MDN.

I think they can be rewritten as regular methods! That won't be too hard. Can do it in another PR although... Do our users really use IE to access the website? Better safe than sorry I guess.

noi5e · 2021-02-02T01:29:04Z

Tests passing, this one's ready to merge... After these prerequisites (in order):

Sagarpreet · 2021-02-02T15:48:47Z

Hey @noi5e , I think you should had kept base branch for PR-2 as PR-1 and for PR-3 as PR-2, so that:

you do not have to remove merge conflicts later if any
easy for reviewer to see the additional changes in the PR

No worries for now, we can follow this approach next time 😄

app/assets/javascripts/editor.js

app/assets/javascripts/post.js

noi5e · 2021-02-02T18:36:30Z

Hey @noi5e , I think you should had kept base branch for PR-2 as PR-1 and for PR-3 as PR-2, so that:

you do not have to remove merge conflicts later if any
easy for reviewer to see the additional changes in the PR

No worries for now, we can follow this approach next time 😄

Thanks Sagarpreet for reading through all these PRs! I know it's complicated. Thanks also for showing me how to change the target merge branch... I wasn't able to do it for these PRs since all the branches are in my forked repository at noi5e/plots2, but still it's really good to know that I can do that in the future.

Thanks again, I know it's hard to read, but I think it will be easier once the earlier PRs are merged and the Files Updated changes.

Again, here's the suggested order of merge in my planning issue: #9069

…#9110 publiclab#9118

jywarren · 2021-02-04T02:42:15Z

These could be timing related, i'll see if they persist after a restart:

Minitest::UnexpectedError:         Capybara::ExpectationNotMet: expected to find css ".comment-preview img" 1 time but there were no matches
            test/system/comment_test.rb:417:in `block (2 levels) in <class:CommentTest>'

=[Screenshot]: tmp/screenshots/failures_test_note:_IMMEDIATE_image_DRAG_&_DROP_into_REPLY_comment_form.png
ERROR CommentTest#test_note:_IMMEDIATE_image_DRAG_&_DROP_into_REPLY_comment_form (214.97s)
Minitest::UnexpectedError:         Capybara::ExpectationNotMet: expected to find css ".comment-preview img" 1 time but there were no matches
            test/system/comment_test.rb:398:in `block (2 levels) in <class:CommentTest>'

[Screenshot]: tmp/screenshots/failures_test_note:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form.png
 FAIL CommentTest#test_note:_IMMEDIATE_image_SELECT_upload_into_EDIT_comment_form (277.47s)
        expected to find css "#comment-preview-edit-1062623862 img" 1 time but there were no matches
        test/system/comment_test.rb:357:in `block (2 levels) in <class:CommentTest>'

[Screenshot]: tmp/screenshots/failures_test_note:_IMMEDIATE_image_SELECT_upload_into_REPLY_comment_form.png
 FAIL CommentTest#test_note:_IMMEDIATE_image_SELECT_upload_into_REPLY_comment_form (340.07s)
        expected to find css "#comment-1062623864-reply-section img" 1 time but there were no matches
        test/system/comment_test.rb:382:in `block (2 levels) in <class:CommentTest>'

====[Screenshot]: tmp/screenshots/failures_test_note:_comment_preview_button_works.png
 FAIL CommentTest#test_note:_comment_preview_button_works (352.89s)
        Expected: "Preview"
          Actual: "Hide Preview"
        test/system/comment_test.rb:91:in `block (2 levels) in <class:CommentTest>'

==[Screenshot]: tmp/screenshots/failures_test_note:_image_DRAG_&_DROP_into_EDIT_form_isn't_cross-wired_with_MAIN_form.png
 FAIL CommentTest#test_note:_image_DRAG_&_DROP_into_EDIT_form_isn't_cross-wired_with_MAIN_form (481.94s)
        Expected: 1
          Actual: 0
        test/system/comment_test.rb:459:in `block (2 levels) in <class:CommentTest>'

[Screenshot]: tmp/screenshots/failures_test_note:_image_SELECT_upload_into_EDIT_form_isn't_CROSS-WIRED_with_MAIN_form.png

noi5e · 2021-02-04T04:41:18Z

I caught the error that was causing the test failures, it was because I was undoing the merge conflicts manually. GitHub thought a line from E.generate_preview was supposed to go in E.toggle_preview, and I let it slide by mistake. Let's see what happens when the tests run again.

====[Screenshot]: tmp/screenshots/failures_test_note:_comment_preview_button_works.png
 FAIL CommentTest#test_note:_comment_preview_button_works (352.89s)
        Expected: "Preview"
          Actual: "Hide Preview"
        test/system/comment_test.rb:91:in `block (2 levels) in <class:CommentTest>'

Hmm, that's weird! That test definitely shouldn't have been running in this PR. That's the new toggle preview test I wrote in #9123, which hasn't been merged yet! I guess it was running at the same time in that PR, so the CI must have gotten confused.

noi5e · 2021-02-04T04:54:31Z

@jywarren Phew, okay that did the trick! Ready to merge again!

noi5e · 2021-02-04T05:34:49Z

app/assets/javascripts/editor.js

+    let previewIDPrefix = "#comment-preview-";
+    if (this.elementIDPrefixes.hasOwnProperty("preview")) {
+      // eg. on /wiki/new & /wiki/edit, the preview element is called #preview-main
+      previewIDPrefix = "#" + this.elementIDPrefixes["preview"] + "-";


This is one of the main changes in this PR. Honestly, I have mixed feelings about it since it's a little weird, especially in the constructor.

But I do think it's an okay workaround. I made this because there was some tension in having editor.js operative on both 1) multi-comment pages and 2) /wiki/new and /wiki/edit.

I wanted preview elements to be named #comment-preview-reply-123 (they used to just be called #preview), for greater transparency. But that clashes with the wiki editor, where there are no comments involved.

So, this is my workaround. It's a little weird, but thanks to OOP, it can be scaled back or revised much easier.

Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

codeclimate · 2021-02-04T05:37:58Z

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

View more on Code Climate.

cesswairimu · 2021-02-04T06:04:18Z

Merging 🎉 🎉 🎉

* merge branches #9104 #9107 #9108 #9110 #9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

* merge branches #9104 #9107 #9108 #9110 #9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test * add text-input class, data-form-id attribute to textarea #9004 #9131 * add data-form-id to save & recover buttons #9004 #9131 * E.setState when clicking save, recover, & .text-input #9004 #9131 * refactor save & recover #9004 #9131 * add icon to recover button #9004 #9131 * restore edit button tooltip #9004 #9131 * save with window.location.pathname, not href #9004 #9131 * new system test for save & recover #9004 #9131 * update test selector #9004 #9131

…lab#9110) * merge editor refactor branches publiclab#9004 * remove dropzone class publiclab#9004 * add new constructor param; write getter methods; change E.method to this.method publiclab#9004 * call new E.textAreaValue getter publiclab#9004 * call new E.constructor; change imagebar IDs publiclab#9004 * change preview ID selector publiclab#9004 * delete typo from merge conflict publiclab#9004 * apply suggestions from code review Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test * add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131 * add data-form-id to save & recover buttons publiclab#9004 publiclab#9131 * E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131 * refactor save & recover publiclab#9004 publiclab#9131 * add icon to recover button publiclab#9004 publiclab#9131 * restore edit button tooltip publiclab#9004 publiclab#9131 * save with window.location.pathname, not href publiclab#9004 publiclab#9131 * new system test for save & recover publiclab#9004 publiclab#9131 * update test selector publiclab#9004 publiclab#9131

…lab#9110) * merge editor refactor branches publiclab#9004 * remove dropzone class publiclab#9004 * add new constructor param; write getter methods; change E.method to this.method publiclab#9004 * call new E.textAreaValue getter publiclab#9004 * call new E.constructor; change imagebar IDs publiclab#9004 * change preview ID selector publiclab#9004 * delete typo from merge conflict publiclab#9004 * apply suggestions from code review Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test * add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131 * add data-form-id to save & recover buttons publiclab#9004 publiclab#9131 * E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131 * refactor save & recover publiclab#9004 publiclab#9131 * add icon to recover button publiclab#9004 publiclab#9131 * restore edit button tooltip publiclab#9004 publiclab#9131 * save with window.location.pathname, not href publiclab#9004 publiclab#9131 * new system test for save & recover publiclab#9004 publiclab#9131 * update test selector publiclab#9004 publiclab#9131

…lab#9110) * merge editor refactor branches publiclab#9004 * remove dropzone class publiclab#9004 * add new constructor param; write getter methods; change E.method to this.method publiclab#9004 * call new E.textAreaValue getter publiclab#9004 * call new E.constructor; change imagebar IDs publiclab#9004 * change preview ID selector publiclab#9004 * delete typo from merge conflict publiclab#9004 * apply suggestions from code review Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test * add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131 * add data-form-id to save & recover buttons publiclab#9004 publiclab#9131 * E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131 * refactor save & recover publiclab#9004 publiclab#9131 * add icon to recover button publiclab#9004 publiclab#9131 * restore edit button tooltip publiclab#9004 publiclab#9131 * save with window.location.pathname, not href publiclab#9004 publiclab#9131 * new system test for save & recover publiclab#9004 publiclab#9131 * update test selector publiclab#9004 publiclab#9131

…lab#9110) * merge editor refactor branches publiclab#9004 * remove dropzone class publiclab#9004 * add new constructor param; write getter methods; change E.method to this.method publiclab#9004 * call new E.textAreaValue getter publiclab#9004 * call new E.constructor; change imagebar IDs publiclab#9004 * change preview ID selector publiclab#9004 * delete typo from merge conflict publiclab#9004 * apply suggestions from code review Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test * add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131 * add data-form-id to save & recover buttons publiclab#9004 publiclab#9131 * E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131 * refactor save & recover publiclab#9004 publiclab#9131 * add icon to recover button publiclab#9004 publiclab#9131 * restore edit button tooltip publiclab#9004 publiclab#9131 * save with window.location.pathname, not href publiclab#9004 publiclab#9131 * new system test for save & recover publiclab#9004 publiclab#9131 * update test selector publiclab#9004 publiclab#9131

noi5e added 6 commits February 1, 2021 12:07

merge editor refactor branches publiclab#9004

5dc442c

remove dropzone class publiclab#9004

1bc9f25

add new constructor param; write getter methods; change E.method to t…

a61264c

…his.method publiclab#9004

call new E.textAreaValue getter publiclab#9004

d1bb02b

call new E.constructor; change imagebar IDs publiclab#9004

8ef8d40

change preview ID selector publiclab#9004

1eabf4f

noi5e requested review from a team as code owners February 2, 2021 00:55

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

noi5e added this to the [Outreachy] Comment Editor Winter 2020 milestone Feb 2, 2021

noi5e mentioned this pull request Feb 2, 2021

Comment Editor Overhaul Project [Outreachy] #9069

Open

noi5e commented Feb 2, 2021

View reviewed changes

noi5e requested review from cesswairimu, jywarren and sagarpreet-chadha February 2, 2021 01:29

Sagarpreet reviewed Feb 2, 2021

View reviewed changes

app/assets/javascripts/editor.js Outdated Show resolved Hide resolved

app/assets/javascripts/post.js Outdated Show resolved Hide resolved

noi5e mentioned this pull request Feb 3, 2021

Disable Drag & Drop Upload on the Image Toolbar Button #9118

Merged

noi5e added a commit to noi5e/plots2 that referenced this pull request Feb 3, 2021

merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab…

854e88c

…#9110 publiclab#9118

noi5e added 2 commits February 3, 2021 16:09

Merge branch 'main' into refactor-editor-constructor

da777d0

Merge branch 'main' into refactor-editor-constructor

01f7fca

jywarren approved these changes Feb 4, 2021

View reviewed changes

Merge branch 'main' into refactor-editor-constructor

6c7ea4d

delete typo from merge conflict publiclab#9004

fde3b9f

noi5e commented Feb 4, 2021

View reviewed changes

apply suggestions from code review

f3dae9f

Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>

cesswairimu merged commit d26b8d2 into publiclab:main Feb 4, 2021

noi5e deleted the refactor-editor-constructor branch February 4, 2021 07:37

jywarren pushed a commit that referenced this pull request Feb 4, 2021

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

2188e0a

* merge branches #9104 #9107 #9108 #9110 #9118 * change #dropzone-large ID to #comment-form-body * add comprehensive preview button test

noi5e mentioned this pull request Feb 6, 2021

[Discussion] Refactor Comment Editor Using OOP #9004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Editor Constructor and Methods, Write Editor Getters #9110

Refactor Editor Constructor and Methods, Write Editor Getters #9110

noi5e commented Feb 2, 2021

gitpod-io bot commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

noi5e Feb 2, 2021

noi5e Feb 2, 2021

noi5e Feb 4, 2021

noi5e Feb 2, 2021

jywarren Feb 4, 2021

jywarren Feb 4, 2021

noi5e Feb 4, 2021

noi5e commented Feb 2, 2021

Sagarpreet commented Feb 2, 2021

noi5e commented Feb 2, 2021

jywarren commented Feb 4, 2021

noi5e commented Feb 4, 2021

noi5e commented Feb 4, 2021

noi5e Feb 4, 2021

codeclimate bot commented Feb 4, 2021

cesswairimu commented Feb 4, 2021

Refactor Editor Constructor and Methods, Write Editor Getters #9110

Refactor Editor Constructor and Methods, Write Editor Getters #9110

Conversation

noi5e commented Feb 2, 2021

gitpod-io bot commented Feb 2, 2021 • edited Loading

codecov bot commented Feb 2, 2021 • edited Loading

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

noi5e commented Feb 2, 2021

Sagarpreet commented Feb 2, 2021

noi5e commented Feb 2, 2021

jywarren commented Feb 4, 2021

noi5e commented Feb 4, 2021

noi5e commented Feb 4, 2021

Choose a reason for hiding this comment

codeclimate bot commented Feb 4, 2021

cesswairimu commented Feb 4, 2021

gitpod-io bot commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading