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

Alerts system #5566

Merged
merged 10 commits into from Apr 3, 2019
Merged

Alerts system #5566

merged 10 commits into from Apr 3, 2019

Conversation

SychO9
Copy link
Contributor

@SychO9 SychO9 commented Apr 1, 2019

This is my proposed PR that closes #4950 and closes #5408 and closes #5412

Additionally alows mods to specify if the alert should be using multiple links instead of one unique link.

through 'show_links' extra array item

Signed-off-by: SychO <sychocouldy@gmail.com>
Signed-off-by: SychO <sychocouldy@gmail.com>
Signed-off-by: SychO <sychocouldy@gmail.com>
Signed-off-by: SychO <sychocouldy@gmail.com>
@live627 live627 added this to the RC3 milestone Apr 1, 2019
Signed-off-by: SychO <sychocouldy@gmail.com>
Sources/Profile-View.php Outdated Show resolved Hide resolved
Signed-off-by: SychO <sychocouldy@gmail.com>
Signed-off-by: SychO <sychocouldy@gmail.com>
Signed-off-by: SychO <sychocouldy@gmail.com>
@SychO9
Copy link
Contributor Author

SychO9 commented Apr 2, 2019

I wonder if it'd be a good idea to use the og_image for an alert that doesn't have a sender avatar
ogloke

@live627
Copy link
Contributor

live627 commented Apr 2, 2019

This didn't really fix #5408 since that link was not modified.

Problem is that the relevant alerts don't use the message id. Perhaps that could be fixed by using new: ?topic=6666.new#new

@SychO9
Copy link
Contributor Author

SychO9 commented Apr 2, 2019

really ? did you try with the PR's changes and then recreate a "X replied to Y " alert and see ? cause I'm pretty positive it fixes it

from what I've noticed, that type of alert has 2 links, one is the poster's, the other is the topic's,
however the alert has a content_link value in its extra db json array, the value of content_link is a correct link containing .new#new at the end, and since this PR prioritizes the value of content_link if it exists, that means that alert takes to the newest post

@live627
Copy link
Contributor

live627 commented Apr 2, 2019 via email

@SychO9
Copy link
Contributor Author

SychO9 commented Apr 2, 2019

Oh this commit should fix that, it basically adds an icon with a link to the alert
6a25c5a

@live627
Copy link
Contributor

live627 commented Apr 3, 2019

that link is generated in fetch_alerts()

3b172fb#diff-01cc3a672ae60fbabf662b0bb5727534R341

Signed-off-by: SychO <sychocouldy@gmail.com>
Copy link
Contributor

@live627 live627 left a comment

Choose a reason for hiding this comment

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

Excellent! I'm ready to merge this if you're finished with.

@live627
Copy link
Contributor

live627 commented Apr 3, 2019

I wonder if it'd be a good idea to use the og_image for an alert that doesn't have a sender avatar

I have no opinion on this. Could @Gwenwyfar weigh in on this?

@SychO9
Copy link
Contributor Author

SychO9 commented Apr 3, 2019

One last thing, I'm seeing a 'icon' for alerts in the template file but nothing that actually creates that item on the source file, unless I'm missing something
https://github.com/SimpleMachines/SMF2.1/blob/e9b4fe9cd17257dc5ec43051eea5d6cb8da83833/Themes/default/Profile.template.php#L112

I wonder if we could ask @Arantor if he remembers anything :p

@Arantor
Copy link
Contributor

Arantor commented Apr 3, 2019

I remember thinking I wanted to support icons but in practice avatars were better so this could really go.

Signed-off-by: SychO <sychocouldy@gmail.com>
@SychO9
Copy link
Contributor Author

SychO9 commented Apr 3, 2019

I remember thinking I wanted to support icons but in practice avatars were better so this could really go.

Oh I see, it can go then thank you

@MissAllSunday MissAllSunday merged commit fc619a4 into SimpleMachines:release-2.1 Apr 3, 2019
@SychO9 SychO9 deleted the alertsSystem branch April 3, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants