-
Notifications
You must be signed in to change notification settings - Fork 555
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
Add analytics events #1036
Add analytics events #1036
Changes from 10 commits
6abe800
47bc647
796c8b8
b5e5b14
dbb2d2a
70fa832
9847d7f
60dc7ca
8d2b035
b1ecd1c
740976a
a42bfdf
17a42ec
c65ca34
36b0cd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,16 @@ import { logIn } from '../../quick-auth/actions'; | |
import { displayName, socialConnections, authButtonsTheme } from '../../connection/social/index'; | ||
|
||
export default class SocialButtonsPane extends React.Component { | ||
handleAuthButtonClick(lock, provider, isSignUp) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this method outside the class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean in the same file, out of the class? |
||
const prov = typeof provider !== 'object' ? provider.toJS() : provider; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need all values of the connection? Can we just send what type of strategy it is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its technically possible to have multiple strategies of the same type though, right? Thats why we were thinking of getting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you specify what properties to keep? I'll work on this in a few minutes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can have multiples connection names per strategy for enterprise (for social it might not be that common) But with connection name and strategy will be enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect, thanks 👍 |
||
|
||
l.emitEvent(lock, 'federated login', { | ||
...prov, | ||
action: isSignUp ? 'signup' : 'signin' | ||
}); | ||
return logIn(l.id(lock), provider); | ||
} | ||
|
||
render() { | ||
// TODO: i don't like that it receives the instructions tanslated | ||
// but it also takes the t fn | ||
|
@@ -31,7 +41,7 @@ export default class SocialButtonsPane extends React.Component { | |
signUp ? 'signUpWithLabel' : 'loginWithLabel', | ||
connectionName || displayName(x) | ||
)} | ||
onClick={() => logIn(l.id(lock), x)} | ||
onClick={() => this.handleAuthButtonClick(lock, x, signUp)} | ||
strategy={x.get('strategy')} | ||
primaryColor={primaryColor} | ||
foregroundColor={foregroundColor} | ||
|
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.
do we need this?