Remove browser notification polyfill and inform user when unsupported #709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
astorije
added
Type: Feature
|
Im running a public version of this PR temporarily for easy testing here: http://avatar.playat.ch:3000/ |
|
Thanks a lot, @MaxLeiter! |
|
Pretty sure there needs to be a check so that notifications can't be created if the api doesn't exist |
|
As far as I can tell, this is the only place where a |
|
I'm thinking there could be potential problem where the option was previously enabled (old version where there was no check, or polyfill removal) |
astorije
force-pushed
the
astorije/rm-notification-poyfill
branch
2 times, most recently
from
3880f81 to
41ba02d
Compare
astorije
force-pushed
the
astorije/rm-notification-poyfill
branch
from
ab6cc01 to
d82a894
Compare
xPaw
approved these changes
Oct 22, 2016
matburnham
pushed a commit
to matburnham/lounge
that referenced
this pull request
Sep 6, 2017
…ion-poyfill Remove browser notification polyfill and inform user when unsupported
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our polyfill is not really a polyfill in the sense that it does not bring similar functionality: it mostly makes the page title blink. I don't even know if that works properly as I haven't seen it in action even once.
I think we should be more explicit than that: if browser supports it, good, use notifications, otherwise, let user know it's not supported and don't try alternative techniques.
According to caniuse.com, we should test this on:
Help welcome with testing.
The testing goal is simply to make sure the app overall works fine apart from notifications in these environments, and that it displays the warning when not supported (though on Chrome for Android for example, it pretends it's supported, even makes the permission request, but no notifications show up...).
Result