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

Post Author notifications UI fixes #519

Closed
wants to merge 6 commits into from
Closed

Conversation

mikeyarce
Copy link
Member

This PR will disable the notifications checkbox for post_authors if:

  • The ef_notification_auto_subscribe_post_author filter is true (default behaviour)
  • If the post_author is currently subscribed (there is an edge case where they might not be)

We also add a visual indication that the user is the Post Author:
Screen Shot 2019-08-22 at 2 33 50 PM

Thoughts on implementation:
My first attempt was to change how the user list is populated in class-module.php, but I quickly realized that looping through each user to programmatically add details is resource-intensive when you have many users. I opted for JavaScript because this way we don't have to loop through each user, we can just target one specific user and modify the elements that way.

Fixes #508

Add  inline JS variables
`post_author_id` - the ID of the post author
`post_author_is_follower` - if the post_author is subscribed to post notifications
`post_author_auto_subscribe` - if post authors are set to be automatically subscribed
`maybe_disable_post_author_checkbox` will disable the post_author checkbox from the Notifications Meta Box if:
1. Post authors are automatically subscribed, and
2. If the post author is currently subscribed

`display_post_author_warning` will add a visual indicator that this user is the Post Author.
@dchymko
Copy link
Contributor

dchymko commented Oct 31, 2019

Tested successfully in conjunction with #540. It makes sense to merge these together, as that will allow users to unsubscribe from their own posts which I don't think we want.

@dchymko dchymko requested a review from mjangda October 31, 2019 22:59
@cojennin
Copy link
Contributor

cojennin commented Nov 5, 2019

Looking good. Couple thoughts:

  1. When using the Block Editor, the disabled checkbox won't appear after the post is saved until after the page is reloaded. Can we observe a post save and trigger the disabled checkbox?
  2. It can also be the case that the current user will be auto-subscribed to the post if the filter ef_notification_auto_subscribe_current_user returns true. Can we add a similar UI treatment for the current user (i.e., show checkbox as disabled when post is saved and add a nice box with something like "Auto-Subscribed")?
  3. I really like this UI treatment, does anyone have any reasons as to why we can't just show it all the time instead of under certain conditions? It adds nice context to the notifications list and it'd be easier to maintain without logical checks

@mjangda mjangda added this to the 0.9.3 milestone Dec 2, 2019
@mjangda
Copy link
Member

mjangda commented Jan 6, 2020

Closing; continued in #563

@mjangda mjangda closed this Jan 6, 2020
@mjangda mjangda deleted the fix-author-notification branch January 6, 2020 16:33
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.

Unsubscribing author from Notifications is impossible: Show error or disable unsubscribe checkbox
4 participants