-
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
Rename dragdrop.js to editorToolbar.js (Outreachy) #9044
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9044 +/- ##
=======================================
Coverage ? 82.05%
=======================================
Files ? 100
Lines ? 5939
Branches ? 0
=======================================
Hits ? 4873
Misses ? 1066
Partials ? 0 |
@jywarren Tests passing, please take a look! Feedback welcome, and this is ready to merge. |
I saw CodeCov was showing |
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 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!!!
Code Climate has analyzed commit aec346c and detected 0 issues on this pull request. View more on Code Climate. |
@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 I still haven't learned how to read the CodeCov output. Looking in the app folder here, but it looks like |
Oh yeah, just to answer your other question:
Linking the comment I left about 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 As long as that's done, the places I've listed will still have eventListeners for 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. |
OK, great, and thanks for your answers! Merging this now!!! 🎉 🎉 🎉 |
* 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
* 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
* 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
* 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
Part of an OOP refactoring of editor.js (See #9004 for more details)
The big change here is that I renamed
dragdrop.js
toeditorToolbar.js
.I did this because this eventListener for
.rich-text-button
s currently lives ineditor.js
:plots2/app/assets/javascripts/editor.js
Lines 1 to 14 in f82a11c
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 newEditor
objects. It makes more sense for adocument.ready
eventListener like the one above to be grouped with similar eventListeners (for.dropzone
s) indragdrop.js
.But if it's combined with
dragdrop.js
, then that script no longer is dealing just with drag & drop listeners, hence the rename toeditorToolbar.js
—this script is basically going to be required everywhere thateditor/toolbar
is present.A plus benefit is that I can move this function to
dragdrop.js
/editorToolbar.js
and deleteeditorHelper.js
:plots2/app/assets/javascripts/editorHelper.js
Lines 2 to 20 in f82a11c
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)