-
Notifications
You must be signed in to change notification settings - Fork 389
Rectified issues in comments for NotificationMessagePayload #879
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
src/messaging.d.ts
Outdated
* | ||
* **Platforms:** Android | ||
* The sound to be played when the device receives the | ||
* notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* The sound to play when the device receives the notification. Supports
* "default" or the filename of a sound resource bundled in the app. Sound files
* must reside in `/res/raw/`.
*
* **Platforms:** Android
*/
I think the docs from #820 might be more suitable here. I will leave @egilmorez to confirm.
Ok since it is already present in #820 . Closing this for now. |
@lahirumaramba Made the changes as expected. Do let me know if anything else is needed. |
src/messaging.d.ts
Outdated
* If specified and a notification with the same tag is already being shown, | ||
* the new notification replaces the existing one in the notification drawer. | ||
* | ||
* The sound to be played when device receives a notification. Supports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using articles everywhere else (instead of the extra-concise style that omits them), we should probably use them here too:
"The sound to be played when the device receives a notification. Supports
* * "default" for the default notification sound of the device"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending! LG with one nit regarding articles.
@egilmorez Sorry for having some semantic/grammatical errors. English isn't my native language. Pushed a commit after adding articles to the comments. Hopefully that fixes your concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @VPanjeta !
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The comments regarding the
NotificationMessagePayload
were not correct. They were placed with different options.The comment related to
tag
was attached tosound
andtag
did not have any comment attached.Had an issue understanding the payload the first time I opened the installed directory to go through the payload of
sendToDevice