feat: add analytic events #75
Conversation
pages/success-banner/index.js
Outdated
| @@ -39,7 +55,9 @@ const Footer = () => ( | |||
| made with | |||
| <img className="icon" src="/static/love.svg" /> | |||
| by | |||
| <img className="brand" src="/static/resin.png" /> | |||
| <a href="https://resin.io/"> | |||
There was a problem hiding this comment.
Should this link have a click-handler too?
There was a problem hiding this comment.
Good catch, I've added that now.
pages/success-banner/index.js
Outdated
| <img className="icon github" src="/static/social/octocat.png" /> | ||
| Star on Github | ||
| </Button> | ||
| <Button href="https://twitter.com/intent/tweet?text=%23etcher"> | ||
| <Button href="https://twitter.com/intent/tweet?text=%23etcher" | ||
| event="click Tweet button"> |
There was a problem hiding this comment.
I wonder if it'd make sense to move the "click " and " button" text into the Button's on-click handler?
So that event="click Tweet button" here could be changed to label="Tweet" and line 24 would change to onClick={ eventLog('click '+this.props.label+' button') }>
| @@ -23,8 +23,7 @@ | |||
| "build": "npm run build:data && npm run build-css && next build", | |||
| "build-css": "node-sass --include-path scss styles/index.scss static/index.css --output-style compressed && node-sass --include-path scss styles/success-banner.scss static/success-banner.css --output-style compressed", | |||
| "serve": "next start", | |||
| "export": "npm run build && next export -o build && cp CNAME build/CNAME && cp .nojekyll build/.nojekyll && npm run copy-static", | |||
| "copy-static": "cp -r static_html/* build/", | |||
| "export": "npm run build && next export -o build && cp CNAME build/CNAME && cp .nojekyll build/.nojekyll", | |||
There was a problem hiding this comment.
Should this simplification of the build-system be mentioned in the commit-message too?
(perhaps it'd be worth splitting this PR into 2 commits? *shrug*)
There was a problem hiding this comment.
I'll mention it, since it was introduced so recently (yesterday) I don't think we need another commit.
static/success-banner.css
Outdated
| @@ -1 +1 @@ | |||
| body{display:flex;justify-content:center;background-color:#535760;font-family:'Helvetica Thin', Helvetica, sans-serif;color:#ffffff}.horizontal,.vertical{display:flex}.vertical{flex-direction:column}.center{display:flex;justify-content:center;align-items:center}.button{display:flex;align-items:center;justify-content:center;width:165px;padding:14px 44px;margin:7.5px;background-color:#313339;color:#fff;font-weight:bold;text-decoration:none;border-radius:3px}.button>img{margin:0 10px}.icon{max-height:24px}.icon.github{margin-right:15px}.icon.twitter{margin-right:8px;margin-left:-4px}footer{margin-top:30px;display:flex;align-items:center;justify-content:center}footer>img{margin:0 0.5em}footer>.brand{max-height:24px}h1{display:inline-block;padding:5px;margin-top:0;font-weight:normal;font-size:26px}.etcher-logo{margin-left:8px;width:130px} | |||
| body{display:flex;justify-content:center;background-color:#535760;font-family:'Helvetica Thin', Helvetica, sans-serif;color:#ffffff}.horizontal,.vertical{display:flex}.vertical{flex-direction:column}.center{display:flex;justify-content:center;align-items:center}.button{display:flex;align-items:center;justify-content:center;width:165px;padding:14px 44px;margin:7.5px;background-color:#313339;color:#fff;font-weight:bold;text-decoration:none;border-radius:3px}.button>img{margin:0 10px}.icon{max-height:24px}.icon.github{margin-right:15px}.icon.twitter{margin-right:8px;margin-left:-4px}footer{margin-top:30px;display:flex;align-items:center;justify-content:center}footer>img,footer>a{margin:0 0.5em}footer>a>.brand{max-height:24px}h1{display:inline-block;padding:5px;margin-top:0;font-weight:normal;font-size:26px}.etcher-logo{margin-left:8px;width:130px} | |||
There was a problem hiding this comment.
Does this file get updated at build / export time? Does it need to be included in the repo?
There was a problem hiding this comment.
It does get updated at build time, yeah. We could probably safely remove it, but the project contains other generated CSS as well, so for the sake of consistency I've kept it.
There was a problem hiding this comment.
Your thoughts here @craig-mulligan ?
There was a problem hiding this comment.
I believe he's away on vacation now (I could be wrong, edit: oh I'm not), but either way I've opened this as a separate issue #76 we can fix later.
|
Thanks for the review @lurch! |
pages/success-banner/index.js
Outdated
| return () => { | ||
| const url = new URL(location.href); | ||
|
|
||
| if (url.searchParams.get('etcher-version').indexOf('1.0.0') !== -1) { |
There was a problem hiding this comment.
Hmmm, what's this for? Isn't Etcher 1.0.0 the only version that contains code to display this banner anyway? Or is this related to the Robot-changes in balena-io/etcher#1422 which were performed after the release of Etcher 1.0.0?
EDIT: And why use indexOf rather than a direct === ? Does using indexOf mean that it'll get confused when we release Etcher 11.0.0 ? ;-) And would this code error if etcher-version didn't exist in the searchParams?
EDIT2: Ah, I guess this is to take the commit-hash versions (created by snapshot-builds) into account. But even then it probably ought to be checking that the string startsWith 1.0.0, rather than checking if it occurs anywhere in the string?
There was a problem hiding this comment.
Yeah, the latter. Prior to those changes they weren't robot objects, and instead just strings forwarded based on which console object method was used. So we have to check and send the appropriate string to Etcher via the console.
There was a problem hiding this comment.
Does this mean that the current 'development' versions of Etcher that we're building and testing with (which include these new Robot-logging changes) ought to have a version-number different to 1.0.0?
Ping @jviotti I guess this is (almost) an example of "API version-breaking" ;)
There was a problem hiding this comment.
If we want to check these kinds of things, we should make Etcher expose a "web banner" version number instead of using Etcher's main version. E.g. we can say "web banner messaging protocol version 1", and then independently increment that when we need to. If such protocol version property is unset, then we infer is the oldest one.
There was a problem hiding this comment.
In this particular case though, we're printing in both sides of the conditional, so I guess it doesn't hurt at all to always send a robot object.
There was a problem hiding this comment.
I guess a webview-version would fit that nicely. So that we'd then send URLs like
https://etcher.io/?etcher-version=1.0.0&webview-version=2
we're printing in both sides of the conditional, so I guess it doesn't hurt at all to always send a robot object.
I wondered that too, but assumed that @Shou had some good reason for not doing so?
There was a problem hiding this comment.
@jviotti So you mean we should just send the robot object instead?
Also I've made balena-io/etcher#1558 now.
pages/success-banner/index.js
Outdated
| } | ||
| } | ||
|
|
||
| class Link extends React.PureComponent { |
There was a problem hiding this comment.
Given that the Button and Link here have target="_blank" I wonder if they should be named e.g. ExternalButton and ExternalLink to emphasise the fact that they'll open a new browser window, rather than being displayed inside the Etcher 'iframe' ?
Or perhaps it makes more sense to leave them as-is (i.e. open-externally by default), and then if we ever wanted Buttons or Links that opened inside the Etcher webview, we could name them e.g. InternalButton and InternalLink?
pages/success-banner/index.js
Outdated
| @@ -22,11 +56,13 @@ const Banner = () => ( | |||
| </h1> | |||
| </div> | |||
| <div className="horizontal center"> | |||
| <Button href="https://github.com/resin-io/etcher"> | |||
| <Button href="https://github.com/resin-io/etcher" | |||
| label="Github star"> | |||
There was a problem hiding this comment.
Minor nit, but for consistency with the actual button label, should this be label="Star on Github" ?
There was a problem hiding this comment.
Sure. I did that now.
pages/success-banner/index.js
Outdated
| <img className="brand" src="/static/resin.png" /> | ||
| <Link href="https://resin.io/" | ||
| label="Resin"> | ||
| <img className="brand" src="/static/resin.png" /> |
There was a problem hiding this comment.
Given that this page is only intended to be displayed inside the Etcher webview, I guess it doesn't really make sense to add ALT attributes to the IMG tags?
There was a problem hiding this comment.
Had the same thought, I'm not sure if it matters either, so I think we can just leave it.
| constructor() { | ||
| super(); | ||
|
|
||
| this.type = 'button'; |
|
@craig-mulligan The most recent commit on this PR caused a Travis build failure, even though the previous commits built fine, and the errors look totally unrelated to what the most recent commit changed. I'll restart the build, in case this was a spurious failure, like we saw in #69 |
pages/success-banner/index.js
Outdated
| @@ -22,11 +57,13 @@ const Banner = () => ( | |||
| </h1> | |||
| </div> | |||
| <div className="horizontal center"> | |||
| <Button href="https://github.com/resin-io/etcher"> | |||
| <Button href="https://github.com/resin-io/etcher" | |||
| label="star on Github"> | |||
There was a problem hiding this comment.
nitpick: should it be Star with a capital S? ;) (as that's what the button says)
There was a problem hiding this comment.
Oh I left it like that because 'Banner ' is prepended. Either works though.
Nope, failed again :-( |
|
:( yea, I'm not sure what it is it only seemed to happen on travis and not locally, I have to run to the airport now, but I'd give the build another try otherwise just merge and I'll fix the false negs when I'm back. |
|
I guess it makes sense to get balena-io/etcher#1558 merged, and then update this PR afterwards. |
|
I've updated this to use the API version information, but the test fails again, at a timeout this time. |
|
@Shou looks like the error is the same as last time (there seems to be a 'knack' to reading NPM errors properly ;-) ). line 21 of fetchData.js says: so I guess this must mean that And then I guess the later |
|
Added some jsdoc, and more importantly fixed a potential future security issue with |
We add analytics sent over the `console.log` method and intercepted by
Etcher. The debug messages are in the [robot][1] format, e.g.
```js
{"command":"log","data":"Hello, World!"}
```
We also remove the recently introduced way to copy Javascript-less pages
into the build because all banners will now contain code for analytics.
[1]: https://github.com/resin-io/etcher/tree/master/lib/shared/robot#the-robot-mechanism
| * @function | ||
| * @private | ||
| * | ||
| * @param {String} eventDecs - event description |
We add analytics sent over the
console.logmethod and intercepted byEtcher. The debug messages are in the robot format, e.g.
We also remove the recently introduced way to copy Javascript-less pages
into the build because all banners will now contain code for analytics.