Better notifications

[gpascualg]

Notifications right now are quite simple, we might improve them by

• Including the message author in the title
• Showing theimage of the forum/author

[gpascualg]

Are notifications working? I don't get them to show, not even the sound. Can someone reproduce?

[jtojnar]

@gpascualg We cannot get them to show either.

[Telematica]

Notification are silent by default, according the JS code here:

Definition:

flarum-ext-chat/js/forum/src/components/ChatFrame.js

Line 406 in 63847ba

forwardMessage(ctrl, message, notify, redraw) {

Implementation:

flarum-ext-chat/js/forum/src/components/ChatFrame.js

Line 196 in 63847ba

this.forwardMessage(ctrl, response.data.attributes.messages[i], false, false);

flarum-ext-chat/js/forum/src/components/ChatFrame.js

Line 251 in 63847ba

this.forwardMessage(ctrl, args.forward[i], false);

I guess we can delegate enabling notifications to the Extension settings on the Admin dashboard... thoughts on this @gpascualg?

[gpascualg]

@Telematica Indeed, default behaviour is muted. We might have an option on the Admin dashboard to change the defaults, it would be a nice addition.

The problem here, however, is that it seems that whether user-enabled or not, the sound doesn't play. Please bear in mind this code (specially notifications/audio) is screaming "refactor me!"...

[Telematica]

@gpascualg I'm interested to be involved on the Flarum development, and its community... I'm starting a new project, and I'm using Flarum as an alternative, hope it soon could be released as stable project (at least 1st version). I will not hesitate to perform a pull request on this. ty!

[gpascualg]

Nice @Telematica, any PR would be much appreciated, I'm really busy right now and can't throw much time into this :(

[gpascualg]

Ah... I've just reviewed the code and you were absolutely right! Regardless of user settings I am always passing false to notify.

We should modify

flarum-ext-chat/js/forum/src/main.js

Line 15 in 63847ba

forward.push(data);

To trigger notifications with this particular data (as they are new messages)

and

flarum-ext-chat/js/forum/src/main.js

Line 36 in 63847ba

forward.push(response.data.attributes.messages[i]);

To disable them (old missatges fetched at start)

Finally,

flarum-ext-chat/js/forum/src/components/ChatFrame.js

Line 251 in 63847ba

this.forwardMessage(ctrl, args.forward[i], false);

would change notify based on those

EDIT: I might have some time to do this! Now that's clear what is failing I can easily solve it

[Telematica]

@gpascualg I'll be issuing a PR soon to fix notification and do the dashboard notification delegation stuff, so you can review it and merge if you think is OK.

[gpascualg]

@Telematica Any news on the PR?

[Telematica]

Sorry, not yet, I've been a bit busy. But a new project is coming; in which I'll need to plug this ext to a plain Flarum instance, so I'll got a chance to make refactors and fixes for this. I'll keep you posted.

[gpascualg]

It's okay! Keep an eye to the issue though, if I grasp a bit of time I might fix it myself, in which case I would close the issue