Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(fcm): Added support for specifying the analytics label for notifications. #597

Merged
merged 7 commits into from Aug 12, 2019
Merged

feat(fcm): Added support for specifying the analytics label for notifications. #597

merged 7 commits into from Aug 12, 2019

Conversation

chemidy
Copy link
Contributor

@chemidy chemidy commented Jul 20, 2019

@hiranya911 hiranya911 assigned egilmorez and chemidy and unassigned egilmorez Jul 22, 2019
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
src/index.d.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez reviewed Jul 22, 2019
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Left some style comments. Thanks for the PR!

egilmorez
egilmorez approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

index.d.ts content looks good, thanks chemidy!

hiranya911
hiranya911 suggested changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks good. Just need some test cases for the new validators.

And I'm also waiting to hear from the FCM team about fcmOptions vs fcm_options in the JSON payload.

test/unit/messaging/messaging.spec.ts Show resolved Hide resolved
hiranya911
hiranya911 approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM

@hiranya911 hiranya911 changed the title Analytics label feat(fcm): Added support for specifying the analytics label for notifications. Aug 12, 2019
@hiranya911 hiranya911 assigned hiranya911 and unassigned chemidy and chong-shao Aug 12, 2019
@hiranya911 hiranya911 merged commit d05dd23 into firebase:master Aug 12, 2019
@chemidy chemidy deleted the analytics_label branch August 15, 2019 01:51
@hiranya911 hiranya911 mentioned this pull request Aug 15, 2019
Closed
@parndt parndt mentioned this pull request Aug 21, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Oct 26, 2019
Open
This was referenced Nov 13, 2019
Closed
Closed
Closed
Closed
This was referenced Mar 10, 2020
Closed
Merged
@snyk-bot snyk-bot mentioned this pull request Apr 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chong-shao chong-shao chong-shao left review comments

@hiranya911 hiranya911 hiranya911 approved these changes

@egilmorez egilmorez egilmorez approved these changes

Assignees

@hiranya911 hiranya911

Labels
release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chemidy @hiranya911 @egilmorez @chong-shao