Adding FCM Notifications #133
Conversation
|
Related #63 |
firebase-messaging-sw.js
Outdated
| storageBucket: "pyconhyd.appspot.com", | ||
| messagingSenderId: "254977934750", | ||
| appId: "1:254977934750:web:9f1ad0357425e5718d828f" | ||
| }; |
|
@aarshad786 test in your local and push the final commit here for the preview. |
|
@aarshad786 This looks good, thanks for implementing this 🎉 Please list out the keys/configuration which we have to update since as of now you are using your firebase creds. |
There was a problem hiding this comment.
Put some comments inline.
Few questions -
- What kind of json data are we sending in payload to display. Can you specify the format here ?
- I see the
appendMessagefunction is commented out. Do you plan to add it to this PR or open a new PR for the UI ? - Will the app constantly listen to the firebase url. Will this have any impact on the general load on the website ?
| importScripts("https://www.gstatic.com/firebasejs/7.5.0/firebase-app.js"); | ||
| importScripts("https://www.gstatic.com/firebasejs/7.5.0/firebase-messaging.js"); | ||
|
|
||
| const firebaseConfig = { |
There was a problem hiding this comment.
This is duplicate code. Can you move it to a separate script and import the messaging variable from there ?
There was a problem hiding this comment.
This is needed for Firebase-messaging-sw which is basically a background listener all the messages coming in
There was a problem hiding this comment.
What I meant is lines 4-14 in this file and line 3-32 in notification.js is duplicte. Please move these lines to separate script and import the messaging variable in both places.
| function showToken(currentToken) { | ||
| // Show token in console and UI. | ||
| const tokenElement = document.querySelector("#token"); | ||
| !!tokenElement && (tokenElement.textContent = currentToken); |
There was a problem hiding this comment.
Is the double ! required here ? document.querySelector returns null if element isn't found
There was a problem hiding this comment.
converting to boolean value using !! to ensure that token exists.. This can be removed but becomes handy when debugging might be required
There was a problem hiding this comment.
Ah so for that shouldn't you be storing !tokenElement in a separate variable and then use it here ? That's how you can debug the variable value for boolean.
| if (!isTokenSentToServer()) { | ||
| console.log("Sending token to server..."); | ||
| // TODO(developer): Send the current token to your server. | ||
| setTokenSentToServer(true); |
There was a problem hiding this comment.
So we are assuming here token is already sent to server ?
There was a problem hiding this comment.
token can be sent to server from here.. This will be great way to send targeted notifications rather sending to everyone
There was a problem hiding this comment.
Thanks for the clarification. Can you mention same in the comment ?
| ); | ||
| // Show permission UI. | ||
| updateUIForPushPermissionRequired(); | ||
| setTokenSentToServer(false); |
There was a problem hiding this comment.
Is this line required ? As per my understanding "sentToServer" should be 1 and it gets set when resetUI() is called after getting permission. Why are we setting it to 0 after that using this function call ?
There was a problem hiding this comment.
We are resetting the variable in storage... once the permission will be received the it is set back again... this is evident in updateUIForPushPermissionRequired() where requestPermission() is called
There was a problem hiding this comment.
Shouldn't this line appear before updateUIForPushPermissionRequired() in that case ?
|
Also it would be good if we can see how the notification looks in different devices i.e in web and the mobile PWA app. |
|
@aarshad786 Let's leave the review comments for now since we need this ready by tomorrow. Just make sure the notification shows correctly. Work on a simple notification display. We need this ready by tomorrow. @bhansa Can you follow up on this ? |
|
Due to time constraint, we were not able to add it in the pyconf website. We will be using this template for HydPy for meetup alerts and future conference websites. |
Adding base code for FCM Notifications
TODO
-Add UI Related code, currently window.alert has been shown
-need to replace firebaseConfig in
firebase-messaging-sw.jsandnotifications.js-use github variables for Firebase Config