Skip to content

feat(fcm): Add image in notification support. - #298

Merged
chong-shao merged 5 commits into
masterfrom
image_in_notification
Aug 27, 2019
Merged

feat(fcm): Add image in notification support.#298
chong-shao merged 5 commits into
masterfrom
image_in_notification

Conversation

@chong-shao

@chong-shao chong-shao commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

Add image in notification support.

Testing

  • Added unit tests and modified one existing integration test to reflect this change.

API Changes

  • In Firebase Cloud Messaging, added "image" field in the general Notification class, the AndroidNotification class, and the ApnsFcmOptions class.

RELEASE NOTE: Added new APIs for specifying an image URL in notifications.

@chong-shao
chong-shao requested a review from hiranya911 August 14, 2019 23:06
@hiranya911 hiranya911 changed the title Add image in notification support. feat(fcm): Add image in notification support. Aug 20, 2019

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just a few suggestions.

Do we need to validate the imageUrl argument? (check not not empty and parse as a valid URL)

Comment thread src/main/java/com/google/firebase/messaging/Notification.java Outdated
Comment thread src/test/java/com/google/firebase/messaging/FirebaseMessagingIT.java Outdated
Comment thread src/test/java/com/google/firebase/messaging/MessageTest.java Outdated
@chong-shao

chong-shao commented Aug 27, 2019

Copy link
Copy Markdown
Contributor Author

I think there's no need to validate the imageUrl argument since the HTTP API also does not do the check.

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants