Skip to content

Conversation

danthedaniel
Copy link
Collaborator

Fixes: #118

Also removed a superfluous call to requestAnimationFrame()

@danthedaniel danthedaniel requested a review from creesch July 7, 2025 02:25
@danthedaniel danthedaniel force-pushed the system-notifications branch from 940a10f to 95d068b Compare July 7, 2025 02:26
@danthedaniel danthedaniel force-pushed the system-notifications branch from 95d068b to a9ba566 Compare July 7, 2025 02:29
Otherwise the border radius pinches repeatedly
@danthedaniel danthedaniel force-pushed the system-notifications branch from e2a23a7 to d4b350a Compare July 7, 2025 02:42
@danthedaniel
Copy link
Collaborator Author

danthedaniel commented Jul 8, 2025

@creesch I could add in a new message type from the client to the server to enable the notification preferences when the user allows notification in the browser.

async function enableNotificationsOnClick() {
    if (await notificationManager.enableNotifications()) {
        console.log('Notifications enabled');
        sendWebsocketMessage('set_preferences', { systemNotifications: true });
    } else {
        console.log('Notifications not enabled');
    }

    document.body.removeEventListener('click', enableNotificationsOnClick);
}

Otherwise users may get confused when they accept the notifications in the browser but don't see them. Or I suppose that might not be an issue if we default to true. It will still be "opt-in".

We could also not have any server-side configuration and rely on the browser's preference configuration.

@creesch
Copy link
Owner

creesch commented Jul 28, 2025

I completely forgot about this PR. We talked about it a little bit in chat. If I remember correctly what we discussed was possibly:

  • Have a settings icon somewhere on the webpage
  • Icon opens a settings window.
  • Settings window has one setting for now "enable notifications in this browser".
  • User clicks that, gets the browser notification permission screen and needs to also allow it there.
  • For the web interface we then simply check on receiving messages if permission has been given.

Having said that, the current way you have implemented it does work nicely. Though to test it on a solo server I had to think for a second how to test it with both the game and chat not in focus. Opening up a second chat in a different browser fixed that :)

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.

Support for the notifications API

2 participants