Fix Rich-Text Cross-Wiring in Comment Forms #9003
Conversation
|
|
noi5e
added
bug
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| 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) { |
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
There was a problem hiding this comment.
Oh yeah, this is important too I guess. The code here is reduplicated from dragdrop.js. I guess, do I make a new asset/js that can be loaded from both scripts?
I'm a little unfamiliar with the conventions for how these scripts are organized and loaded, can you give me some pointers?
| // 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) { |
There was a problem hiding this comment.
Feels weird to have this click handler in editor.js, but I wasn't sure where else to put it.
| $E.initialize(params); | ||
| const action = e.currentTarget.dataset.action // 'bold', 'italic', etc. | ||
| $E[action](); // call the appropriate editor function | ||
| }); |
There was a problem hiding this comment.
These lines replace the inline onclick handlers that used to be in the _toolbar.html.erb partial.
Codecov Report
@@ Coverage Diff @@
## main #9003 +/- ##
=======================================
Coverage ? 74.54%
=======================================
Files ? 100
Lines ? 6058
Branches ? 0
=======================================
Hits ? 4516
Misses ? 1542
Partials ? 0 |
| link: function() { | ||
| uri = prompt('Enter a URL'); | ||
| if (uri == null) { uri = ""; } | ||
| $E.wrap('[', '](' + uri + ')'); | ||
| }, |
There was a problem hiding this comment.
I added a check for null values here. Before, if the user didn't fill out the prompt, ()[null] would appear in the comment form.
| class="bold-button btn btn-outline-secondary btn-sm" | ||
| onClick="$E.bold()" | ||
| data-toggle="tooltip" | ||
| class="bold-button btn btn-outline-secondary btn-sm rich-text-button" | ||
| > |
There was a problem hiding this comment.
Again, note that I removed the onclick function. I also added a data-action attribute which is used in the button click handler.
| <% end %> | ||
| </div> | ||
| </div> | ||
| <% end %> |
There was a problem hiding this comment.
Unrelated to this PR, but I've been wanting to change this indentation for a while and thought I'd bundle it into this one.
| text_input_value = main_comment_form.find('#text-input').value | ||
| assert_equal(text_input_value, '****') | ||
| end | ||
|
|
There was a problem hiding this comment.
Two new tests, these were the most likely scenarios for screw-ups when I did my manual testing.
noi5e
force-pushed
the
bug/rich-text-crosswiring
branch
2 times, most recently
from
09461ac
to
fa81084
Compare
noi5e
force-pushed
the
bug/rich-text-crosswiring
branch
from
933447f
to
fa81084
Compare
|
I did a |
noi5e
changed the title
Fixes #8478
The review comments explain most everything. I think this fix covers most bases, since
$D.selectedis now assigned any time a button is clicked.Interestingly, even before I merged unique button IDs, I saw that
e.targetwas pretty accurate. That makes me think that these cross-wiring fixes maybe have less to do with unique IDs for every single element, and more to do with how the jQuery eventListeners are attached? Still not sure.The most important thing here is code architecture, which codeclimate caught.
dragdrop.jsandeditor.jsnow have duplicated code... Not sure where to put their common code. Also, not sure if theclickeventHandler truly belongs ineditor.js. Pointers definitely welcome!!! 🙏(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)