Implement opt-in analytics with cookie bar #1906
Conversation
As hopefully all explained in comments. Fixes element-hq/element-web#6727
| @@ -353,7 +354,9 @@ const LoggedInView = React.createClass({ | |||
|
|
|||
| let topBar; | |||
| const isGuest = this.props.matrixClient.isGuest(); | |||
| if (this.props.hasNewVersion) { | |||
| if (this.props.showCookieBar) { | |||
| topBar = <CookieBar />; | |||
There was a problem hiding this comment.
Annoying that this will prevent the display of the password nag bar. I don't have a fix to suggest for this though, :/
There was a problem hiding this comment.
Not showing it until the cookie bar is dismissed doesn't seem like the end of the world.
src/settings/Settings.js
Outdated
| supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, | ||
| displayName: _td('Show cookie bar'), | ||
| default: true, | ||
| }, |
There was a problem hiding this comment.
Will this give us a separate settings toggle saying, "Show cookies bar"? Either way, don't we just need to show it if analyticsOptIn === null?
There was a problem hiding this comment.
It will. I'll try and remove it but keep it as a setting.
| @@ -227,8 +229,6 @@ export default React.createClass({ | |||
| componentWillMount: function() { | |||
| SdkConfig.put(this.props.config); | |||
|
|
|||
| if (!SettingsStore.getValue("analyticsOptOut")) Analytics.enable(); | |||
There was a problem hiding this comment.
Are we deliberately re-prompting people who've already opted out? Might be nice to propagate the old analyticsOptOut to analyticsOptIn.
There was a problem hiding this comment.
Yes, everyone needs to see the cookie bar. This just gives them a chance to make the decision again if they've already made it.
Also ignore any account data events that aren;t widgets.
Wait for echo from server when adding user widgets
| @@ -213,11 +213,15 @@ export const SETTINGS = { | |||
| supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, | |||
| default: "en", | |||
| }, | |||
| "analyticsOptOut": { | |||
| "analyticsOptIn": { | |||
| supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG, | |||
There was a problem hiding this comment.
Something to consider may be to prevent the configuration from setting this flag. WITH_CONFIG means the config.json is able to set a default value that overrides this.
There are arguments for both keeping and ditching the config-level option, just thought I'd point it out.
There was a problem hiding this comment.
This is as intended, allowing config.json to override both seems like a legitimate use case.
| <a | ||
| className="mx_MatrixToolbar_link" | ||
| target="_blank" | ||
| href="https://riot.im/privacy" |
There was a problem hiding this comment.
Yep, absolutely.
We were previously waiting for them to appear which is silly if we were deleting them.
Wait for deletion of widgets as well addition
Also: - Make CookieBar ES6 class - Alter phrasing on CookieBar - Conditionaly display longer "...(See our ++cookie and privacy policies++)." if policy link is configured.
…react-sdk into luke/opt-in-analytics
|
What's the status with the 'show cookie bar' setting? I'm not sure I really understand why this is a thing. |
It was a convenient way to add a flag that was tracked in local storage and in config. Some deploys might want to disable the cookie bar (maybe they already have a cookie warning somewhere else). (Also, no need to re-assign for a response on a comment) |
|
Right, I think I understand the reasoning behind this now, in that what I was missing was:
|
No description provided.