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.

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

Rename dragdrop.js to editorToolbar.js (Outreachy) #9044

Merged
merged 7 commits into from
Jan 20, 2021

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Jan 19, 2021

Part of an OOP refactoring of editor.js (See #9004 for more details)

The big change here is that I renamed dragdrop.js to editorToolbar.js.

I did this because this eventListener for .rich-text-buttons currently lives in editor.js:

// jQuery (document).ready function:
$(function() {
// this click eventHandler assigns $D.selected to the appropriate comment form
// on pages with multiple comments, $D.selected needs to be accurate so that rich-text changes (bold, italic, etc.) go into the right comment form
// however, the editor is also used on pages with JUST ONE form, and no other comments, eg. /wiki/new & /wiki/edit, so this code needs to be reusable for that context
$('.rich-text-button').on('click', function(e) {
const { textArea, preview, dSelected } = getEditorParams(e.target); // defined in editorHelper.js
// assign dSelected
if (dSelected) { $D.selected = dSelected; }
$E.setState(textArea, preview);
const action = e.currentTarget.dataset.action // 'bold', 'italic', etc.
$E[action](); // call the appropriate editor function
});
});

This is confusing, especially since if I'm going to refactor editor.js for OOP, I want that script to be a class definition for new Editor objects. It makes more sense for a document.ready eventListener like the one above to be grouped with similar eventListeners (for .dropzones) in dragdrop.js.

But if it's combined with dragdrop.js, then that script no longer is dealing just with drag & drop listeners, hence the rename to editorToolbar.js—this script is basically going to be required everywhere that editor/toolbar is present.

A plus benefit is that I can move this function to dragdrop.js/editorToolbar.js and delete editorHelper.js:

const getEditorParams = (targetDiv) => {
const closestCommentFormWrapper = targetDiv.closest('div.comment-form-wrapper'); // this returns null if there is no match
let params = {};
// there are no .comment-form-wrappers on /wiki/edit or /wiki/new
// these pages just have a single text-input form.
if (closestCommentFormWrapper) {
params['dSelected'] = $(closestCommentFormWrapper);
// assign the ID of the textarea within the closest comment-form-wrapper
params['textarea'] = closestCommentFormWrapper.querySelector('textarea').id;
params['preview'] = closestCommentFormWrapper.querySelector('.comment-preview').id;
} else {
// default to #text-input
// #text-input ID should be unique, and the only comment form on /wiki/new & /wiki/edit
params['textarea'] = 'text-input';
// #preview-main should be unique as well
params['preview'] = 'preview-main';
}
return params;
};

NOTE: There are a bunch of places which historically depend on dragdrop.js/editorToolbar.js for the drag & drop functionality, and I've listed them in this comment.


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

@noi5e noi5e added JavaScript feature explains that the issue is to add a new feature outreachy labels Jan 19, 2021
@noi5e noi5e requested review from a team as code owners January 19, 2021 20:48
@gitpod-io
Copy link

gitpod-io bot commented Jan 19, 2021

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9044   +/-   ##
=======================================
  Coverage        ?   82.05%           
=======================================
  Files           ?      100           
  Lines           ?     5939           
  Branches        ?        0           
=======================================
  Hits            ?     4873           
  Misses          ?     1066           
  Partials        ?        0           

@noi5e
Copy link
Contributor Author

noi5e commented Jan 19, 2021

@jywarren Tests passing, please take a look! Feedback welcome, and this is ready to merge.

@jywarren
Copy link
Member

I saw CodeCov was showing No coverage uploaded for pull request base and realized it was still configured for a master branch; fixed this and restarting tests to get a better coverage report!

@jywarren jywarren closed this Jan 20, 2021
@jywarren jywarren reopened this Jan 20, 2021
@gitpod-io
Copy link

gitpod-io bot commented Jan 20, 2021

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This looks great -- excellent work! To what degree are all these changes covered by tests, do you think? Have you looked into CodeCov to see what the code coverage is for the /app/assets/javascripts/FOO.js files touched here is?

Thanks!!!

@jywarren jywarren changed the title Rename dragdrop.js to editorToolbar.js Rename dragdrop.js to editorToolbar.js (Outreachy) Jan 20, 2021
@codeclimate
Copy link

codeclimate bot commented Jan 20, 2021

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

View more on Code Climate.

@noi5e
Copy link
Contributor Author

noi5e commented Jan 20, 2021

@jywarren Just fixed the merge conflict, so this one's ready-to-merge again.

I took a look at CodeCov, but it looks like it's currently still saying No coverage uploaded for pull request base.
Screen Shot 2021-01-20 at 10 54 27 AM

I still haven't learned how to read the CodeCov output. Looking in the app folder here, but it looks like assets doesn't appear for some reason? Any thoughts?

@noi5e noi5e requested a review from jywarren January 20, 2021 18:59
@noi5e
Copy link
Contributor Author

noi5e commented Jan 20, 2021

Oh yeah, just to answer your other question:

To what degree are all these changes covered by tests, do you think?

Linking the comment I left about dragdrop.js coverage again.

For this particular PR, I don't think there will be any significant problems because I'm just renaming the script and not changing the contents significantly. For this PR, what matters is that I make sure that every instance of javascript_include_tag: 'dragdrop' is changed to editorToolbar.

As long as that's done, the places I've listed will still have eventListeners for .dropzone or #side-dropzone which is what matters.

I think it is important to have system tests in place for the legacy editor (which we're conversing about in #8858), in case the contents of this script change in the future.

@jywarren
Copy link
Member

OK, great, and thanks for your answers! Merging this now!!! 🎉 🎉 🎉

@jywarren jywarren merged commit 38e37eb into publiclab:main Jan 20, 2021
@noi5e noi5e deleted the rename-dragdrop-to-toolbar branch January 21, 2021 18:37
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* remove editorHelper publiclab#9004

* deleted editorHelper & moved function to editorToolbar publiclab#9004

* moved rich-text eventListener to editorToolbar.js publiclab#9004

* rename dragdrop.js to editorToolbar.js
publiclab#9004

* rename dragdrop.js to editorToolbar.js

* rename dragdrop.js to editorToolbar.js
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* remove editorHelper publiclab#9004

* deleted editorHelper & moved function to editorToolbar publiclab#9004

* moved rich-text eventListener to editorToolbar.js publiclab#9004

* rename dragdrop.js to editorToolbar.js
publiclab#9004

* rename dragdrop.js to editorToolbar.js

* rename dragdrop.js to editorToolbar.js
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* remove editorHelper publiclab#9004

* deleted editorHelper & moved function to editorToolbar publiclab#9004

* moved rich-text eventListener to editorToolbar.js publiclab#9004

* rename dragdrop.js to editorToolbar.js
publiclab#9004

* rename dragdrop.js to editorToolbar.js

* rename dragdrop.js to editorToolbar.js
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* remove editorHelper publiclab#9004

* deleted editorHelper & moved function to editorToolbar publiclab#9004

* moved rich-text eventListener to editorToolbar.js publiclab#9004

* rename dragdrop.js to editorToolbar.js
publiclab#9004

* rename dragdrop.js to editorToolbar.js

* rename dragdrop.js to editorToolbar.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature explains that the issue is to add a new feature JavaScript outreachy readytomerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants