Skip to content

Adds follower feedback in comments/replies as part of #270#271

Closed
goodguyry wants to merge 8 commits intoAutomattic:masterfrom
goodguyry:follower-feedback-270a-comments
Closed

Adds follower feedback in comments/replies as part of #270#271
goodguyry wants to merge 8 commits intoAutomattic:masterfrom
goodguyry:follower-feedback-270a-comments

Conversation

@goodguyry
Copy link
Copy Markdown
Contributor

Indicates whether or not any users or user groups were selected to receive a notification of a new comment (the first part of #270). Feedback is given both in the comment/reply form and in the comment itself (on hover, like the reply link). The indicator in the comment persists, for transparency's sake, when/if Edit Flow Notifications are turned off.

I split #270 up into two pull requests - I believe them to be two separate features.

@goodguyry
Copy link
Copy Markdown
Contributor Author

It occurred to me that some screenshots would be helpful...

No one is selected to receive notification of this comment:

edit-flow-comment-form-message-no-followers

The indicated users are selected to be notified of this comment:

edit-flow-comment-form-message

Notifications were turned off when this comment was created:

edit-flow-comments-hover-3

Notifications were enabled, but no one was selected when this comment was created:

edit-flow-comments-hover-1

The indicated users were selected to be notified when this comment was created:

edit-flow-comments-hover-2

@jerclarke
Copy link
Copy Markdown
Contributor

Wow these both look fantastic according to the screenshots! Just what I was looking for an then some. My users would definitely make less mistakes.

Too bad about the "Travis CI biuld filed" part. I'll have to look up what that entails. A quick look at the patches and the main thing I noticed was that none of the strings were internationalized (gettext/i18n), do you know how to set them up to use _()?

@goodguyry
Copy link
Copy Markdown
Contributor Author

I'm glad you like them. I'll look into getting those strings set up correctly.

@cojennin cojennin added type: enhancement New feature or request dev-feedback labels May 20, 2016
@cojennin
Copy link
Copy Markdown
Contributor

cojennin commented Jun 7, 2016

@goodguyry would you still be interested in working on this PR? Seems like useful functionality to have. Would be great to refine this PR a bit and get the tests working again.

@goodguyry
Copy link
Copy Markdown
Contributor Author

Sure. I'll find time to get reacquainted and see what I can do.

@cojennin
Copy link
Copy Markdown
Contributor

cojennin commented Sep 6, 2016

@goodguyry Awesome, let me know if you need a hand with getting reacquainted!

@WPprodigy
Copy link
Copy Markdown
Contributor

WPprodigy commented Apr 5, 2018

Did a quick review and some testing. All seems to be working good still and as intended 👍 . I'm going to close out this PR shortly and open up a new one to build on top of what's been done here - just posting a couple comments below for what stuck out that will need to be done.

editorial-comments.php

$_POST['notification'] needs to be sanitized before saving to the db in comment meta. Also should check if it isset first before accessing.

Will need to escape the output in the get_comment_notification_meta function since $notification is technically coming from an external place (the db).

To better fit into the current templating, it would be best to rename the above function to comment_notification_message and just return the string. Then the HTML markup can be with all the other HTML where the function is called.

Would probably be good to introduce a filter to disable this new functionality as well, in case it won't be useful to a site and they don't want extra unneeded DB entries.

Lastly, instead of 1 being the default/empty value for the comment meta - I'm thinking it would be best to just not add comment meta at all. Then we can just check if the response is empty/false when trying to retrieve it. This will help prevent unnecessary DB additions.


editorial-comments.js

Checking if the value is No one will be notified won't work out if the string is translated, so would be good to avoid needing to do that. Also ideally the default value won't be 1 but rather just empty. So this conditional will be able to be simplified.

The and in the list of subscribers along with the No one will be notified text will need to be translatable. So will need to pass them through on the PHP side when the script is localized.

Would be good to avoid .html(), so instead we can just be a bit more specific about where the text is being pulled from.

Minor thing, but the property notify seemed a bit confusing to me at first, not sure if that verb is a good fit for what the function is doing. Considering renaming to perhaps notifiedSubscribers or notifiedMessage. Maybe just to picky on my part though though ;)

notifications.js

Rather than initiating the function in this file, I'm thinking it would be better to keep it all together in editorial-comments.js and instead just watch for notification settings changes. To do this, could perhaps just emit a custom event and watch for that to be called.

@WPprodigy
Copy link
Copy Markdown
Contributor

WPprodigy commented Apr 8, 2018

Closing here and continuing in #452. Thanks for your contribution :)

@WPprodigy WPprodigy closed this Apr 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants