-
Notifications
You must be signed in to change notification settings - Fork 4k
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
New Feature: Response Templates #5419
New Feature: Response Templates #5419
Conversation
@rhymes I've updated the PR with your requests; did not go through with refactoring the JS CodeClimate issues after all. I'd rather leave that for a separate PR. Rest of it should be okay, but looks like I might need to update the test coverage. 😩 Anyway feel free to review or not dependent on your priorities -- I added you since you've been reviewing this whole time and have the context. :) |
Hi @Zhao-Andy, let me know if you want to address the first request for change (the internal dashboard is a bit broken) or you'd rather do it in a following PR, don't know how important it is that it's not fully working:
|
Ooooh this is basically what we were trying to do when we added |
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.
Starting to look good. Just some changes to the frontend when you have a chance. Other things to consider are unit tests in the frontend if it brings more confidence and consider adding JS Doc comments to functions. We're slowly improving things in the frontend, so baby steps ;)
One other thing to note. The PR is quite large. I realize the ERBs and JS need to go together. Just wondering if anything in this PR can be pulled out into another PR.
app/assets/javascripts/initializers/initializeCommentsPage.js.erb
Outdated
Show resolved
Hide resolved
document.getElementById("image-upload-file-label-" + randomIdNumber).innerHTML = errorMessage; | ||
document.getElementById("image-upload-file-label-" + randomIdNumber).style.color = '#e05252'; | ||
document.getElementById("image-upload-submit-" + randomIdNumber).style.display = 'none'; | ||
document.getElementById("image-upload-file-label-" + randomIdNumber).innerHTML = errorMessage + '<button id="warningclosebutt">×</button>'; |
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.
If these IDs are the same as the ones above, you can create references to these elements and reuse the reference.
</div>\ | ||
</form>'; | ||
} | ||
|
||
function cancel(event, el) { | ||
event.preventDefault(); | ||
replaceActionButts(el.parentNode.parentNode.parentNode.parentNode); | ||
replaceActionButts(el.parentNode.parentNode.parentNode.parentNode.parentNode); |
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.
What is el.parentNode.parentNode.parentNode.parentNode.parentNode
referencing? There may be a better way to get a reference to it. If not, consider creating a variable so it can be described and/or consider a comment explaining what it refers to.
@@ -561,7 +579,7 @@ | |||
font-size: 0.9em; | |||
} | |||
.github-repo-featured-indicator { |
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.
I can't find a reference to this CSS selector anywhere in the code except for this file. Are you sure it's used?
import loadingImg from '../../assets/images/loading-ellipsis.svg' | ||
import { handleLoggedOut, fetchResponseTemplates, addReplyObservers, addToggleListener } from '../responseTemplates/responseTemplates'; | ||
|
||
const userStatus = document.querySelector('body').getAttribute('data-user-status'); |
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.
const userStatus = document.querySelector('body').getAttribute('data-user-status'); | |
const { userStatus } = document.body.dataset; |
|
||
button.addEventListener('click', () => { | ||
const responsesWrapper = document.querySelector('.mod-responses-container'); | ||
responsesWrapper.style.display = 'flex'; |
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.
Can this display: flex
be added to a CSS class instead? Less JS and in a CSS class it gets cached.
@@ -0,0 +1,17 @@ | |||
import { handleLoggedOut, addReplyObservers } from '../responseTemplates/responseTemplates'; | |||
|
|||
const userStatus = document.querySelector('body').getAttribute('data-user-status'); |
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.
Same as above
const userStatus = document.querySelector('body').getAttribute('data-user-status'); | |
const { userStatus } = document.body.dataset; |
/* eslint-disable no-alert */ | ||
/* eslint-disable no-restricted-globals */ | ||
function buildModResponseHTML(response) { | ||
const array = response |
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.
consider a more descriptive name for the variable instead of the generic array
.
${modResponseHTML} | ||
<header><h3>Personal Response Templates</h3></header> | ||
${personalResponseHTML} | ||
<a target="_blank" rel="noopener nofollow" href="/settings/response-templates" class="mod-response-create-new">Create new template</a> |
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.
+1 for rel="noopener nofollow"
. 👏🏻
|
||
const observer = new MutationObserver(callback); | ||
|
||
targetNodes.forEach(node => { |
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.
With this, n target nodes get observed which could be quite expensive. It may make more sense to observe the parent node of all the target nodes.
Co-Authored-By: Nick Taylor <nick@iamdeveloper.com>
Closing in favor of splitting this into smaller PRs. |
What type of PR is this? (check all applicable)
Description
This adds the canned response feature to be used by anyone as well as moderators. Canned responses are templates that users can save and reuse to reply to a comment.
Moderators can use canned responses made by the DEV Team to leave a comment as Sloan. These "moderator canned responses" are meant to be used to help reply in situations where posting under Sloan might be more effective than as the moderator themselves.
To do:
user_moderator_id
to productionRelated Tickets & Documents
#5000
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?