-
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
Refactor editor.js with Object-Oriented Principles #9104
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9104 +/- ##
=======================================
Coverage ? 82.14%
=======================================
Files ? 100
Lines ? 5936
Branches ? 0
=======================================
Hits ? 4876
Misses ? 1060
Partials ? 0 |
// default parameters reference the IDs of: | ||
// 1. main comment form in multi-comment wikis, questions, & research notes. | ||
// 2. the only editor form on /wiki/new and /wiki/edit | ||
constructor(textarea = "text-input", preview = "comment-preview-main", title = "title") { |
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.
These default params are just a stepping stone for now.
Since the element IDs are all standardized (or exist, or are finally unique) now, it's easier to do a rewrite so that constructor takes just one parameter. For example:
$E = new Editor('reply-1234');
means that the editor object has access to both #comment-preview-reply-1234
and #text-input-reply-1234
.
Much simpler.
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.
Nice !!
// code seems unused, commenting out for now. | ||
// is_editing = function() { | ||
// return ($E.textarea[0].selectionStart == 0 && $E.textarea[0].selectionEnd == 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.
Couldn't find this code used anywhere doing a search for is_editing
in VSCode. Will delete in the next few PRs if the tests keep passing.
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.
Let's remove and clean this then 💯
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.
Deleted it in another PR!
app/assets/javascripts/editor.js
Outdated
// we only refresh $E's values if we are on a page using the rich-text editor (most pages). | ||
// the legacy editor pages only have one editor form, unlike pages with multiple comments. | ||
if (this.isRichTextEditor(window.location.pathname)) { this.refresh(); } | ||
wrap(a, b, args) { |
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.
Function wrap
has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
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.
Yikes, I remember codeclimate was giving me a lot of grief in this PR as well.
I did a little bit of work in the wrap
method (mostly renaming methods and writing new comments), which started triggering this error. Then I completely scaled it back, removing all the changes, and still got the error. Weird.
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 passing now.
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.
Great! We don't ALWAYS have to agree with CC on every suggestion. But i think long term it makes for better code, so I really appreciate it!
// default parameters reference the IDs of: | ||
// 1. main comment form in multi-comment wikis, questions, & research notes. | ||
// 2. the only editor form on /wiki/new and /wiki/edit | ||
constructor(textarea = "text-input", preview = "comment-preview-main", title = "title") { |
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.
Nice !!
$E.textarea.val($E.textarea.val().substring(0,start) + replace + $E.textarea.val().substring(end,len)); | ||
}, | ||
bold: function() { | ||
wrap(a, b, newlineDesired = false, fallback) { |
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.
can we name it better than a
and b
for new contributors to understand this better?
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 totally agree about renaming a
and b
!!! I'll do it soon.
// code seems unused, commenting out for now. | ||
// is_editing = function() { | ||
// return ($E.textarea[0].selectionStart == 0 && $E.textarea[0].selectionEnd == 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.
Let's remove and clean this then 💯
break; | ||
default: | ||
$E.wrap('<a href="'+data.result.url.split('?')[0]+'"><i class="fa fa-file"></i> ','</a>', {'newline': true, 'fallback': data.result['filename'].replace(/[()]/g , "-")}) // on its own line; see /app/assets/js/editor.js |
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.
Why was fallback used? and why is it removed now?
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 the only place where fallback
is still used (3 times):
plots2/app/assets/javascripts/editorToolbar.js
Lines 108 to 118 in 97247e3
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; | |
case 'csv': | |
$E.wrap('[graph:' + file_url + ']', '', {'newline': true}) | |
break; | |
default: | |
$E.wrap('<a href="'+data.result.url.split('?')[0]+'"><i class="fa fa-file"></i> ','</a>', {'newline': true, 'fallback': data.result['filename'].replace(/[()]/g , "-")}) // on its own line; see /app/assets/js/editor.js | |
} |
I didn't delete fallback
here, but I just separated it out into two parameters instead of one object:
- Before:
$E.wrap(a, b, { fallback: "fallback", newline: "true" });
- After:
$E.wrap(a, b, true, "fallback")
;`
In other words, wrap
now takes four parameters, instead of three. And none of the parameters are objects.
I know it might be a little harder to read but I really needed to do this to pass codeclimate. It was failing this PR saying that the old code in $E.wrap
had too much 'cognitive complexity'. Part of the reason why was because the parameter was an object, and we had to do a check like const fallBackExists = args && args['fallback']
—which adds to cognitive complexity.
tldr; I didn't delete fallback, the same functionality is still there. Just changed parameters.
@@ -2,7 +2,7 @@ jQuery(document).ready(function() { | |||
|
|||
$('.datepicker').datepicker() | |||
|
|||
$E.initialize() | |||
$E = new Editor(); |
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.
🎉
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.
🎉 🎉 🎉 niceeeeee
$("img").lazyload(); | ||
});</script> | ||
<script> | ||
var comments_length = <%= @node.comments.length %>; |
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 not getting used anywhere, right?
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.
comments_length
? I think it still is here:
plots2/app/assets/javascripts/question.js
Lines 13 to 23 in 97247e3
if (check_matched) { | |
$('.answer-0-comments').show(); | |
} else { | |
$('.answer-0-comments').slice(-3).show(); | |
if (comments_length > 3) { | |
$('#answer-0-comment').prepend('<p id="answer-0-expand" style="color: #006dcc;">View ' + comment_select(0).length + ' previous comments</p>'); | |
$("#answer-0-expand").on('click', function() { | |
expand_comments(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.
But I don't know what this code does 😅
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 like old code related to the "Answers" system which we've deprecated; answers were back turned into comments. It seems it was for showing only the first... 3 answers, hiding the rest behind a "show more" kind of thing? I think that's all gone now, so we should be good to remove it.
Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>
Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com>
@jywarren @Sagarpreet ready to merge again! |
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 know this looks like a lot of commits, but most of them were just to pass codeclimate. Is it just me or did it get really strict all of a sudden?
Hmm. i wonder if this is just an old and messy code file and we had previously dismissed CC suggestions, but re-editing the file is bringing them all back up?
This looks super though, thank you! Let's remember to circle back to other thoughts here like removing segments, in a follow-up PR; @noi5e if you can drop them into an issue for now that'd be great!
* 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
* object-oriented refactor! publiclab#9004 * create instances of new object-oriented editor publiclab#9004 * pass codeclimate: add semicolon; comment unused methods publiclab#9004 * move editor instantiation to notes/comments partial publiclab#9004 * adding const to pass codeclimate publiclab#9004 * reformat object methods publiclab#9004 * instantiate 1 editor object each on wikis, notes, and questions publiclab#9004 * rewrite to reduce cognitive complexity publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * hopefully the last codeclimate rewrite publiclab#9004 * rewrite E.wrap's parameters to pass codeclimate publiclab#9004 * remove newlines to appease codeclimate publiclab#9004 * typo: highlighT instead of highligh publiclab#9004 * fixing typo publiclab#9004 * change const to let publiclab#9004 * assign textarea & preview to jQuery objects in constructor publiclab#9004 * change newText assignment Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> * change uri check for null publiclab#9004 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
* object-oriented refactor! publiclab#9004 * create instances of new object-oriented editor publiclab#9004 * pass codeclimate: add semicolon; comment unused methods publiclab#9004 * move editor instantiation to notes/comments partial publiclab#9004 * adding const to pass codeclimate publiclab#9004 * reformat object methods publiclab#9004 * instantiate 1 editor object each on wikis, notes, and questions publiclab#9004 * rewrite to reduce cognitive complexity publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * hopefully the last codeclimate rewrite publiclab#9004 * rewrite E.wrap's parameters to pass codeclimate publiclab#9004 * remove newlines to appease codeclimate publiclab#9004 * typo: highlighT instead of highligh publiclab#9004 * fixing typo publiclab#9004 * change const to let publiclab#9004 * assign textarea & preview to jQuery objects in constructor publiclab#9004 * change newText assignment Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> * change uri check for null publiclab#9004 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
* object-oriented refactor! publiclab#9004 * create instances of new object-oriented editor publiclab#9004 * pass codeclimate: add semicolon; comment unused methods publiclab#9004 * move editor instantiation to notes/comments partial publiclab#9004 * adding const to pass codeclimate publiclab#9004 * reformat object methods publiclab#9004 * instantiate 1 editor object each on wikis, notes, and questions publiclab#9004 * rewrite to reduce cognitive complexity publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * hopefully the last codeclimate rewrite publiclab#9004 * rewrite E.wrap's parameters to pass codeclimate publiclab#9004 * remove newlines to appease codeclimate publiclab#9004 * typo: highlighT instead of highligh publiclab#9004 * fixing typo publiclab#9004 * change const to let publiclab#9004 * assign textarea & preview to jQuery objects in constructor publiclab#9004 * change newText assignment Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> * change uri check for null publiclab#9004 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
* object-oriented refactor! publiclab#9004 * create instances of new object-oriented editor publiclab#9004 * pass codeclimate: add semicolon; comment unused methods publiclab#9004 * move editor instantiation to notes/comments partial publiclab#9004 * adding const to pass codeclimate publiclab#9004 * reformat object methods publiclab#9004 * instantiate 1 editor object each on wikis, notes, and questions publiclab#9004 * rewrite to reduce cognitive complexity publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * trying to pass codeclimate publiclab#9004 * hopefully the last codeclimate rewrite publiclab#9004 * rewrite E.wrap's parameters to pass codeclimate publiclab#9004 * remove newlines to appease codeclimate publiclab#9004 * typo: highlighT instead of highligh publiclab#9004 * fixing typo publiclab#9004 * change const to let publiclab#9004 * assign textarea & preview to jQuery objects in constructor publiclab#9004 * change newText assignment Co-authored-by: Sagarpreet <53554917+Sagarpreet@users.noreply.github.com> * change uri check for null publiclab#9004 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
See #9004
I think that went a lot easier than I expected! This seems to work okay testing in the local environment, lets see how the system tests do. Will update this comment if there's anything significant.
UPDATE:
Ready to merge! 🙌
I know this looks like a lot of commits, but most of them were just to pass codeclimate. Is it just me or did it get really strict all of a sudden?
(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)