-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: add more interface for flutter web support #444
Conversation
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 PR title seems not in semantic format
@@ -1473,6 +1515,7 @@ AmplitudeClient.prototype.logEventWithGroups = function ( | |||
groups, | |||
opt_callback, | |||
opt_error_callback, | |||
outOfSession = false, |
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.
Is this one in group also used in flutter? Or used in other SDK?
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.
Flutter doesn't have the logEventWithGroups
method. But I think those functions are exposed to Amplitude-Javascript user, so it's better to keep them consistent. What do you think?
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.
@yuhao900914 For other SDKs we have, do we have similaroutOfSession
support on logEventWithGroups
?
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.
Andorid and iOS are similar, the logEvent function is able to pass groups info and outOfSession together.
- (void)logEvent:(NSString *)eventType withEventProperties:(NSDictionary *)eventProperties withApiProperties:(NSDictionary *)apiProperties withUserProperties:(NSDictionary *)userProperties withGroups:(NSDictionary *)groups withGroupProperties:(NSDictionary *)groupProperties withTimestamp:(NSNumber *)timestamp outOfSession:(BOOL)outOfSession {xxx}
a81bc71
to
579b3be
Compare
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, just some minor changes
- Should remove library/version code and tests since we already have those features
- Some typos
- Tests look like they are failing
Would also be good if you could list APIs in the PR summary for future reference 😄
966eebb
to
9632966
Compare
8d42516
to
7c22f21
Compare
7c22f21
to
fc4d624
Compare
0e1b116
to
05245f9
Compare
Summary
Expose more interfaces for flutter web support
The following are the API added in this pr.
Checklist