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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adac028
add rich-text tests #8478
noi5e Jan 13, 2021
d969585
delete onclick for rich-text buttons; add data-action attribute #8478
noi5e Jan 13, 2021
91e6937
fixed indentation #8478
noi5e Jan 13, 2021
fa81084
add click handler for rich-text buttons #8478
noi5e Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions app/assets/javascripts/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,28 @@
// 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels weird to have this click handler in editor.js, but I wasn't sure where else to put it.

const closestCommentFormWrapper = e.target.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) {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

$D.selected = $(closestCommentFormWrapper);
// assign the ID of the textarea within the closest comment-form-wrapper
params['textarea'] = closestCommentFormWrapper.querySelector('textarea').id;
} else {
// default to #text-input
// ideally there should only be one per page
params['textarea'] = 'text-input';
}
$E.initialize(params);
const action = e.currentTarget.dataset.action // 'bold', 'italic', etc.
$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.

These lines replace the inline onclick handlers that used to be in the _toolbar.html.erb partial.

});

$E = {
initialize: function(args) {
args = args || {}
Expand Down Expand Up @@ -65,9 +90,10 @@ $E = {
italic: function() {
$E.wrap('_','_')
},
link: function(uri) {
uri = uri || prompt('Enter a URL')
$E.wrap('[',']('+uri+')')
link: function() {
uri = prompt('Enter a URL');
if (uri == null) { uri = ""; }
noi5e marked this conversation as resolved.
Show resolved Hide resolved
$E.wrap('[', '](' + uri + ')');
},
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 added a check for null values here. Before, if the user didn't fill out the prompt, ()[null] would appear in the comment form.

image: function(src) {
$E.wrap('\n![',']('+src+')\n')
Expand Down
26 changes: 13 additions & 13 deletions app/views/editor/_toolbar.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,34 +14,34 @@
</div>
<!-- defined in application_helper.rb -->
<% toolbar_element_id = get_toolbar_element_id(location, local_assigns[:reply_to], local_assigns[:comment_id]) %>
<div class="btn-group mr-2 ">
<div class="btn-group mr-2">
<a
id="bold-button-<%= toolbar_element_id %>"
data-toggle="tooltip"
title="Bold"
data-action="bold"
data-placement="bottom"
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"
>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, note that I removed the onclick function. I also added a data-action attribute which is used in the button click handler.

<i class="fa fa-bold" ></i>
</a>
<a
id="italic-button-<%= toolbar_element_id %>"
data-toggle="tooltip"
title="Italic"
data-action="italic"
data-placement="bottom"
class="italic-button btn btn-outline-secondary btn-sm"
onClick="$E.italic()"
data-toggle="tooltip"
class="italic-button btn btn-outline-secondary btn-sm rich-text-button"
>
<i class="fa fa-italic" ></i>
</a>
<a
id="header-button-<%= toolbar_element_id %>"
data-toggle="tooltip"
title="Header"
data-action="h2"
data-placement="bottom"
class="header-button btn btn-outline-secondary btn-sm"
onClick="$E.h2()"
data-toggle="tooltip"
class="header-button btn btn-outline-secondary btn-sm rich-text-button"
>
<b>
<span class="d-md-none d-lg-inline">
Expand All @@ -52,11 +52,11 @@
<a
id="link-button-<%= toolbar_element_id %>"
aria-label="Make a link"
data-toggle="tooltip"
title="Make a link"
data-action="link"
data-placement="bottom"
class="link-button btn btn-outline-secondary btn-sm" href="javascript:void(0)"
onClick="$E.link()"
data-toggle="tooltip"
class="link-button btn btn-outline-secondary btn-sm rich-text-button" href="javascript:void(0)"
>
<i class="fa fa-link"></i>
</a>
Expand Down
42 changes: 20 additions & 22 deletions app/views/notes/_comment.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,27 +72,26 @@
</a>
<% end %>
<% if current_user %>
<button style="border: 1px solid;padding: 4px 6px;" class="btn btn-outline-secondary dropdown" type="button" id="dropdownMenuButton" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
<i class='icon fa fa-smile-o'></i>
<sup style="font-size: 12px;">+</sup>
</button>

<div id="emoji-dropdown" style="background:#f8f8f8;padding:0;" class=" dropdown-menu dropdown-menu-right" aria-labelledby="dropdownMenuButton">
<p id="emoji-title" style="text-align: center;color: #586069; font-size:14px;margin:6px;">Pick your reaction</p>
<div id= "dropdown-divider" style="display: block;height: 0;border-top: 1px solid #e1e4e8;"></div>

<div id="emoji-list" class="emoji-container">
<% emoji_names, emoji_image_map = emoji_info %>
<% emoji_names.each do |e| %>
<% capitalized_emoji_name = e.split("-").map(&:capitalize).join %>
<% str = "/comment/like?comment_id=#{comment.cid}&user_id=#{current_user.uid}&emoji_type=#{capitalized_emoji_name}" %>
<%= link_to str, data: { method: "post", remote: true }, style: "margin: 6px;" do %>
<img alt="React to post" class="emoji grow" height="20" width="20" src="<%= emoji_image_map[e] %>">
<% end %>
<% end %>
</div>
</div>
<% end %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

<button style="border: 1px solid;padding: 4px 6px;" class="btn btn-outline-secondary dropdown" type="button" id="dropdownMenuButton" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
<i class='icon fa fa-smile-o'></i>
<sup style="font-size: 12px;">+</sup>
</button>
<div id="emoji-dropdown" style="background:#f8f8f8;padding:0;" class=" dropdown-menu dropdown-menu-right" aria-labelledby="dropdownMenuButton">
<p id="emoji-title" style="text-align: center;color: #586069; font-size:14px;margin:6px;">Pick your reaction</p>
<div id= "dropdown-divider" style="display: block;height: 0;border-top: 1px solid #e1e4e8;">
</div>
<div id="emoji-list" class="emoji-container">
<% emoji_names, emoji_image_map = emoji_info %>
<% emoji_names.each do |e| %>
<% capitalized_emoji_name = e.split("-").map(&:capitalize).join %>
<% str = "/comment/like?comment_id=#{comment.cid}&user_id=#{current_user.uid}&emoji_type=#{capitalized_emoji_name}" %>
<%= link_to str, data: { method: "post", remote: true }, style: "margin: 6px;" do %>
<img alt="React to post" class="emoji grow" height="20" width="20" src="<%= emoji_image_map[e] %>">
<% end %>
<% end %>
</div>
</div>
<% end %>
</div>
</div>

Expand Down Expand Up @@ -153,7 +152,6 @@
<div class="alert alert-info">Have you attempted or completed this activity? Consider <a href="/post?n=15798&tags=replication:<%= comment.nid %>,<%= comment.parent.tagnames.join(',') %>">sharing how it went</a> to help refine and improve it.</div>
<% end %>


<% if comment.reply_to.nil? %>
<% comment.replied_comments.order("timestamp ASC").each do |replied_comment| %>
<%= render :partial => "notes/comment", :locals => {:comment => replied_comment} %>
Expand Down
31 changes: 31 additions & 0 deletions test/system/comment_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,37 @@ def get_path(page_type, path)
assert_selector('.btn[data-original-title="Help"]', count: 1)
end

test "#{page_type_string}: IMMEDIATE rich-text input works in MAIN form" do
visit get_path(page_type, nodes(node_name).path)
main_comment_form = page.find('h4', text: /Post comment|Post Comment/).find(:xpath, '..') # title text on wikis is 'Post comment'
main_comment_form.find("[data-original-title='Bold']").click
text_input_value = main_comment_form.find('#text-input').value
assert_equal(text_input_value, '****')
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two new tests, these were the most likely scenarios for screw-ups when I did my manual testing.

test "#{page_type_string}: rich-text input into REPLY form isn't CROSS-WIRED with EDIT form" do
nodes(node_name).add_comment({
uid: 5,
body: comment_text
})
nodes(node_name).add_comment({
uid: 2,
body: comment_text
})
visit get_path(page_type, nodes(node_name).path)
# open up the edit comment form
page.find("#edit-comment-btn").click
# find the EDIT id
edit_comment_form_id = page.find('h4', text: 'Edit comment').find(:xpath, '..')[:id]
# open up the reply comment form
page.all('p', text: 'Reply to this comment...')[1].click
page.all("[data-original-title='Bold']")[0].click
reply_input_value = page.find('[id^=text-input-reply-]').value
edit_input_value = page.find('#' + edit_comment_form_id + ' textarea').value
assert_equal(comment_text, edit_input_value)
assert_equal('****', reply_input_value)
end

test "#{page_type_string}: edit comment" do
nodes(node_name).add_comment({
uid: 2,
Expand Down