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

For #483 - Updates notifications js/php to only show users who will be notified #492

Merged

Conversation

jerclarke
Copy link
Contributor

@jerclarke jerclarke commented Jan 9, 2019

I've now added all three major changes needed to ensure only users who will be sent a notification are listed in the Editorial Comments "Notification list".

No Access and No Email badges both work using a unified system now, and those users aren't added to the "Notified list".

The current user's checkbox is marked with a special class, and the "Notification list" JS is set up to ignore the current user.

Text below describes the original commit.

For #483 - Updates notifications js/php to avoid "No Access" users showing in the notified list in editorial comments

Adds .post_following_list-no_access to the "No Access" warning spans (in two places), then uses that during "Notified list" generation to exclude people without access from the list (which already happens during email sending, this just cleans up the display so it matches the actual outcome).

My intention is to add similar tests for current_user and no_email users.

  • notifications.php - add .post_following_list-no_access to the initial PHP-created "No Access" span
  • notifications.js - add .post_following_list-no_access to the post-AJAX JS-created "No Access" span
  • notifications.js - Move the triggering of following_list_updated() to AFTER the no access stuff runs, so that it can see the span. Previously the "notified list" was updated before, so it couldn't see any of the results of the access test
  • editorial-comments.js - before inserting a user into the "notified list" array, check if a span indicating a problem is next to the checkbox

…" users showing in the notified list in editorial comments

Adds  .post_following_list-no_access to the "No Access" warning spans (in two places), then uses that during "Notified list" generation to exclude people without access from the list (which already happens during email sending, this just cleans up the display so it matches the actual outcome).

My intention is to add similar tests for current_user and no_email users.

- notifications.php - add .post_following_list-no_access to the initial PHP-created "No Access" span
- notifications.js - add .post_following_list-no_access to the post-AJAX JS-created "No Access" span
- notifications.js - Move the triggering of following_list_updated() to AFTER the no access stuff runs, so that it can see the span. Previously the "notified list" was updated before, so it couldn't see any of the results of the access test
- editorial-comments.js - before inserting a user into the "notified list" array, check if a span indicating a problem is next to the checkbox
Updates previous changes to handle users with no email address along with those who lack capability-level access to the post.

Generalizes the "no access badge" functions to be "warning badges":

Renames php method display_subscriber_access_badge() to be display_subscriber_warning_badges()
Renames js function toggle_no_access_badge() to be toggle_warning_badges()

notifications.php
- rename method as described above
- add 'No Email' string for localization
- When preparing AJAX reply, evaluate each subscribed user for "no email" warning along with "no access" evaluation
- When adding span for "no access" also add "no email" in a similar way

notifications.js
- rename function as described above
- re-organize evaluation of "no access" to be part of general toggle_warning_badges() function
- also evaluate AJAX output for subscribers_with_no_email

editorial-comments.js
- update notifiedMessage() to ignore users with either the  "no access" or the "no email" flags
@jerclarke
Copy link
Contributor Author

Second commit generalizes the warning badge system and adds support for "No Email" badge.

  • Evaluates users for having an email or not during AJAX
  • Returns a list of users with no email
  • Checks that list when generating display
  • Checks for the email class before adding subscribers to notified list

Final fix for the "notified list", which avoids having the current user displayed as a recipient because they won't be notified (they are sending the message after all).

class-module.php
- add a CSS class to the checkbox for the "current user" so we can check it later and not show yourself in notified list

editorila-comments.js
- during notifiedMessage() skip users who's checkbox has the class
@jerclarke
Copy link
Contributor Author

Third commit is the final fix for #483, fixing the "notified list".

This change avoids having the current user displayed as a recipient because they won't be notified (they are sending the message after all).

class-module.php

  • add a CSS class to the checkbox for the "current user" so we can check it later and not show yourself in notified list

editorila-comments.js

  • during notifiedMessage() skip users who's checkbox has the class

@jerclarke jerclarke changed the title For #483 - Updates notifications js/php to avoid "No Access" users sh… For #483 - Updates notifications js/php to only show users who will be notified Jan 9, 2019
@rinatkhaziev
Copy link
Contributor

@jerclarke This looks great, I only have a minor note, can we make the colors of badges match the official colors: https://make.wordpress.org/design/handbook/design-guide/foundations/colors/#grays or https://make.wordpress.org/design/handbook/design-guide/foundations/colors/#hues

I'd personally pick one of grays, but I defer to you.

This is somewhat important because as it is right now, the plugin is full of arbitrary colors and a bit off-brand, and I'd like to change that situation or at the very least not to aggravate it :)

@jerclarke
Copy link
Contributor Author

jerclarke commented Jan 10, 2019

@rinatkhaziev I would love to! The color there now wasn't my choice and I was going to propose a follow up change to make it higher contrast, that faint red doesn't have enough contrast to pass any kind of accessibility test.

Will do a patch right now

- Previously the "No Access" and "No Email" badges were a pale red that wasn't contrasty enough and wasn't a WP standard.
- Now they match the red from https://make.wordpress.org/design/handbook/design-guide/foundations/colors/#hues
- I also updated the selectors to be specifically the two badges currently in use, rather than applying to any span
@jerclarke
Copy link
Contributor Author

screen shot 2019-01-10 at 11 03 23 am

Here's how it looks after the patch. Ugly but appropriate, since it's important that they notice the warnings, and the amount of contrast is much more accessible!

Sidenote: I think we should also change the "green flash" when a subscription is saved (upon AJAX return) to be a "red flash" when there's a warning, but haven't looked into what needs to be changed for that yet.

@rinatkhaziev
Copy link
Contributor

@jerclarke

Looking good! Merging and then I'll be spending the rest of the day doing chores like updating the changelog and readme, but this definitely goes out today.

Thanks for your work both on code and planning/raising concerns.

@rinatkhaziev rinatkhaziev merged commit 19a9680 into Automattic:master Jan 10, 2019
@jerclarke
Copy link
Contributor Author

Thanks! So glad this new version will be out there!

@jerclarke jerclarke deleted the feature/483-accurate-notified-list branch January 10, 2019 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants