-
Notifications
You must be signed in to change notification settings - Fork 144
Implemented FCM Webpush API #16
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
| /// The direction in which to display the notification. | ||
| /// </summary> | ||
| [JsonIgnore] | ||
| public Direction? Direction { get; set; } |
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.
I'm wondering why you have a set of nullable properties for types like this? Why not use default values instead? To me it seems like a pain to deal with the nullability of e.g bool properties rather than just reading the default value.
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.
This is to make sure those parameters get dropped from the JSON output. For most parameters, we want to avoid sending a default value to the backend (0 for ints, false for bools etc) when they are not set by the user.
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.
It does worry me a little that we're letting optimizations leak into the API surface. I guess if that's the way you do it across the board it's ok to me.
bklimt
left a comment
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.
LGTM. I'll leave the review to Stewart.
WebpushConfigandWebpushNotificationtypesIEnumerable.Copy())