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

Fix Rich-Text Cross-Wiring in Comment Forms #9011

Merged
merged 7 commits into from Jan 17, 2021

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Jan 14, 2021

Fixes #8478

This PR is a duplicate of #9003, which is now closed (I did a git rebase main in the other branch, which was bundling changes from main into the PR. Couldn't figure out how to undo that).

From the other PR write-up:

Interestingly, even before I merged unique button IDs, I saw that e.target was 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.


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

@noi5e noi5e requested review from a team as code owners January 14, 2021 21:26
@gitpod-io
Copy link

gitpod-io bot commented Jan 14, 2021

@noi5e noi5e changed the title Bug: Fix Rich-Text Cross-Wiring Bug: Fix Rich-Text Cross-Wiring in Comment Forms Jan 14, 2021
@codeclimate
Copy link

codeclimate bot commented Jan 14, 2021

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

View more on Code Climate.

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9011   +/-   ##
=======================================
  Coverage        ?   82.03%           
=======================================
  Files           ?      100           
  Lines           ?     5933           
  Branches        ?        0           
=======================================
  Hits            ?     4867           
  Misses          ?     1066           
  Partials        ?        0           

$E[action](); // call the appropriate editor function
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have doubts about if this is the right place to put this click eventHandler.

@@ -32,20 +32,10 @@ jQuery(function() {
$('#side-dropzone').removeClass('hover');
});
$('.dropzone').on('drop',function(e) {
// this 'drop' listener is also reused for pages with just one form, ie. /wiki/new
const closestCommentFormWrapper = e.target.closest('div.comment-form-wrapper'); // this returns null if there is no match
const params = getEditorParams(e.target); // defined in editorHelper.js
e.preventDefault();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I doing this right: calling on editorHelper.js? I browsed the Rails guide to JS assets, but am not sure if what I'm doing falls under 'best practices'. Would appreciate some pointers if you have any.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that looks fine to me

@noi5e noi5e added the bug the issue is regarding one of our programs which faces problems when a certain task is executed label Jan 14, 2021
@noi5e
Copy link
Contributor Author

noi5e commented Jan 15, 2021

@jywarren Had some questions about requiring JS files in Rails, and where to put a click eventHandler.

Other than that, this is ready to merge!

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

🎉

@@ -32,20 +32,10 @@ jQuery(function() {
$('#side-dropzone').removeClass('hover');
});
$('.dropzone').on('drop',function(e) {
// this 'drop' listener is also reused for pages with just one form, ie. /wiki/new
const closestCommentFormWrapper = e.target.closest('div.comment-form-wrapper'); // this returns null if there is no match
const params = getEditorParams(e.target); // defined in editorHelper.js
e.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that looks fine to me

Copy link
Collaborator

@cesswairimu cesswairimu left a comment

Choose a reason for hiding this comment

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

Fantastic 🎉

@cesswairimu cesswairimu merged commit 68c9022 into publiclab:main Jan 17, 2021
@noi5e noi5e deleted the bug/fix-rich-text-crosswiring branch January 17, 2021 17:41
@noi5e noi5e changed the title Bug: Fix Rich-Text Cross-Wiring in Comment Forms Fix Rich-Text Cross-Wiring in Comment Forms Jan 23, 2021
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* add rich-text tests publiclab#8478

* fix indentation publiclab#8478

* delete onclick for rich-text buttons; add data-action attribute publiclab#8478

* add click handler for rich-text buttons publiclab#8478

* extract code to editorHelper for DRY publiclab#8478

* require editorHelper.js publiclab#8478

* change == to === publiclab#8478
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* add rich-text tests publiclab#8478

* fix indentation publiclab#8478

* delete onclick for rich-text buttons; add data-action attribute publiclab#8478

* add click handler for rich-text buttons publiclab#8478

* extract code to editorHelper for DRY publiclab#8478

* require editorHelper.js publiclab#8478

* change == to === publiclab#8478
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* add rich-text tests publiclab#8478

* fix indentation publiclab#8478

* delete onclick for rich-text buttons; add data-action attribute publiclab#8478

* add click handler for rich-text buttons publiclab#8478

* extract code to editorHelper for DRY publiclab#8478

* require editorHelper.js publiclab#8478

* change == to === publiclab#8478
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* add rich-text tests publiclab#8478

* fix indentation publiclab#8478

* delete onclick for rich-text buttons; add data-action attribute publiclab#8478

* add click handler for rich-text buttons publiclab#8478

* extract code to editorHelper for DRY publiclab#8478

* require editorHelper.js publiclab#8478

* change == to === publiclab#8478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug the issue is regarding one of our programs which faces problems when a certain task is executed outreachy readytomerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong text input gets selected while using toolbar on comments
3 participants