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

Update front.html.twig Add border to non federated notifications for admins and moderators #1297

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TheVillageGuy
Copy link
Contributor

Add border to non federated notifications for admins and moderators

This allows admins/moderators (when they receive notifications in the future) to quickly see if there has been any activity by local users on their instance

image

Add border to non federated notifications for admins and moderators

This allows admins/moderators (when they receive notifications in the future) to quickly see if there has been any activity by local users on their instance
@melroy89 melroy89 added enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end labels Dec 18, 2024
@melroy89
Copy link
Member

I'm not so sure about this change. Not that I'm not in favor to make it clear whether it's federated or non-federated notification. It's more about how to solve this issue.

I believe adding a border would only add confusing. Only you now know what this means, but its not clear at all what a border means around a notification at all.

If you would try to distinguish between local or federated messages. I believe we should solve it differently. Maybe with an icon + alt text or something, not a "random" border.

@TheVillageGuy
Copy link
Contributor Author

I'm not so sure about this change. Not that I'm not in favor to make it clear whether it's federated or non-federated notification. It's more about how to solve this issue.

I believe adding a border would only add confusing. Only you now know what this means, but its not clear at all what a border means around a notification at all.

If you would try to distinguish between local or federated messages. I believe we should solve it differently. Maybe with an icon + alt text or something, not a "random" border.

I can add an alt text, that's a great idea. I've been using this for a while now and it works really well for me. The border is subtle but still catches my attention. I think it works better than an icon, I know from experience icons can be missed when skimming lists. These borders are hard to miss

@TheVillageGuy
Copy link
Contributor Author

TheVillageGuy commented Dec 22, 2024

image

Added tooltip with username. Don't remember who but someone said it would be nice to be able to see which instance something was posted from. 2 birds with one stone. I don't feel this should be iconified. (Ignore the 'comment' it's for testing purposes)

Title shows entry as local and author
</div>

{% for notification in notifications %}
<div class="{{ html_classes('section section--small notification', {'opacity-50': notification.status is not same as 'new' }) }}">
<div class="{{ html_classes('section section--small notification', {'opacity-50': notification.status is not same as 'new' }) }}"
{% if not notification.entry.magazine.apId and not notification.post.magazine.apId and (app.user.admin or app.user.moderator) %}
Copy link
Member

@melroy89 melroy89 Dec 22, 2024

Choose a reason for hiding this comment

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

did you also tested on if the variable is not-set / null or empty?? And if these checks are not causing any 503 errors?

(eg. when you are not an admin or something like that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I tested the lot and it's live on my instance, the variables are always set, which surprised me. Check me in mbin development as there is one weird situation with comments

Copy link
Member

Choose a reason for hiding this comment

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

Yes I tested the lot and it's live on my instance, the variables are always set, which surprised me.

Well, because you are an admin? Try to login with a regular account?

Copy link
Contributor Author

@TheVillageGuy TheVillageGuy Dec 23, 2024

Choose a reason for hiding this comment

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

Yes I tested the lot and it's live on my instance, the variables are always set, which surprised me.

Well, because you are an admin? Try to login with a regular account?

I did, and if the user is not an admin they never reach the code. I'm stil looking at moderators, it is not working as I had hoped.

I always test everything with a regular account after updates and when changing things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants