feat(avatars): add metrics for avatar cropping operations #2591

eoger · 2015-06-12T23:02:41Z

pdehaan · 2015-06-12T23:06:44Z

bower.json

@@ -27,7 +27,8 @@
    "JavaScript-MD5": "1.1.0",
    "jquery-ui-touch-punch": "https://github.com/zaach/jquery-ui-touch-punch.git#89e5a9159494de37df1e09cccf9036dda5429830",
    "blueimp-canvas-to-blob": "2.1.0",
-    "mailcheck": "https://github.com/mailcheck/mailcheck.git#9fcf9c40da06f6769e5d702dd0fd1f33f36e55a6"
+    "mailcheck": "https://github.com/mailcheck/mailcheck.git#9fcf9c40da06f6769e5d702dd0fd1f33f36e55a6",
+    "jquery-simulate": "~1.0.1"


Not sure if you want to drop the semver range operator ~ here. For consistency.

Good catch, I added the dependency with bower install --save this is why, I'll correct it right away

EXCUSES!

Actually, I think that Bower (like npm) has a --save-exact flag which drops that range operator.
bower/bower#1654

pdehaan · 2015-06-12T23:25:54Z

app/tests/spec/lib/cropper.js
  line 87  col 31  'event' is defined but never used.
✖ 1 problem
Warning: Task "jshint:app" failed.� Use --force to continue.
Aborted due to warnings.

pdehaan · 2015-06-12T23:39:20Z

app/tests/spec/lib/cropper.js


      beforeEach(function () {
+        mockMetrics = {
+          logEvent: function (event) {


This unused event here was causing the JSHint error/warning that broke the Travis build. Since it isn't doing anything here, we can probably just drop the event parameter and the logEvent() is just a noop.

eoger · 2015-06-12T23:41:46Z

I'm gonna git hook commit "grunt lint", this is getting ridiculous.

pdehaan · 2015-06-12T23:50:06Z

But... then what will I do with all these GIFs?

We could use something like https://www.npmjs.com/package/pre-commit or https://www.npmjs.com/package/husky and make it a devDep so everybody is forced into the precommit route.

IIRC, we used husky (with grunt-newer so it'd only lint modified files instead of all files) in https://github.com/mozilla/chronicle/ and then just ran the custom lint task on prepush.

I can't remember if @shane-tomlinson mentioned that he had a local precommit hook, or if @zaach or @vladikoff use one.

UPDATE: Filed as #2592 so we can hug it out during the next triage meeting.

shane-tomlinson · 2015-06-14T10:48:06Z

app/scripts/lib/cropper.js

@@ -71,22 +74,28 @@ function (_) {
    this.slider.on('input', function () {
      self.resize(parseInt(this.value, 10));
    });
+    this.slider.on('change', function () {
+      self.metrics.logEvent('avatar-cropper.zoom.range');


@zaach - IIRC, you wanted to keep this particular module pretty "pure" so that it could be extracted into its own library. Adding metrics will destroy that purity since metrics are very specific to our app.

If we are going to integrate this more fully with the app, I think we should turn it into a Backbone based view and use all the functionality available to us to reduce boilerplate, if not, then let's try to move the metrics out of here somehow.

Yes, good point. The view could pass in an event emitter (e.g. an object extended with Backbone event mixin) that listens for events and forwards them to our metrics. If the cropper receives such an object it'll trigger the events on that.

Yes, good point. The view could pass in an event emitter (e.g. an object extended with Backbone event mixin) that listens for events and forwards them to our metrics. If the cropper receives such an object it'll trigger the events on that.

I can deal with that. Another options is to allow a consumer to pass in individual callbacks for rotate, zoom out and zoom in. The callbacks would log the events. Instead of requiring the mediator between DOM events and metrics be an EventEmitter, the mediator between the two would just be functions.

Ah yes, the tried and true JavaScript callback. 👍

eoger · 2015-06-16T22:36:48Z

Ready for another review 👍

shane-tomlinson · 2015-06-17T13:58:59Z

@eoger - rebase please!

eoger · 2015-06-17T14:32:32Z

Done 👍

zaach · 2015-06-17T18:16:15Z

app/scripts/lib/cropper.js

+    this.onTranslate = options.onTranslate ? options.onTranslate : _.noop;
+    this.onZoomIn = options.onZoomIn ? options.onZoomIn : _.noop;
+    this.onZoomOut = options.onZoomOut ? options.onZoomOut : _.noop;
+    this.onZoomRangeChange = options.onZoomRangeChange ? options.onZoomRangeChange : _.noop;


Nit: you could use || instead of ternary operators here.

Good one thank you!

zaach · 2015-06-17T18:30:52Z

app/scripts/views/settings/avatar_crop.js

@@ -102,6 +107,26 @@ function (p, Cocktail, FormView, SettingsMixin, AvatarMixin, Template,
          self.navigate('settings');
          return result;
        });
+    },
+
+    onRotate: function () {


Since these methods are only used internally they can begin with _.

eoger · 2015-06-18T21:40:55Z

PR corrected, ty zaach

zaach · 2015-06-18T22:40:39Z

Looks like it needs another rebase, then it should be good to go.

eoger · 2015-06-18T22:46:09Z

Yep "use strict" again, corrected!

shane-tomlinson · 2015-06-21T10:43:20Z

app/scripts/views/settings/avatar_crop.js

+    },
+
+    _onRotate: function () {
+      this.logScreenEvent('rotate.cw');


These events need to be added to the docs.

shane-tomlinson · 2015-06-21T11:18:24Z

@eoger - can you rebase & add the requested docs?

eoger · 2015-06-22T17:22:25Z

Done!

vladikoff · 2015-06-29T17:20:31Z

@eoger sorry needs a rebase, there is a bower.json change that we need to alphabetize.

eoger · 2015-06-29T17:44:42Z

Rebased and ready to merge.

shane-tomlinson · 2015-07-01T13:11:52Z

app/scripts/lib/cropper.js

   * }
   */
  function Cropper (options) {

+    /*jshint maxcomplexity:10 */


jshint is gone. This line is no longer needed.

shane-tomlinson · 2015-07-01T13:15:48Z

Removed the jshint line and manually merged in commit f0f917e.

Thanks @eoger!

eoger added the waffle:needs review label Jun 12, 2015

pdehaan reviewed Jun 12, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 864c42d to f8d9ecd Compare June 12, 2015 23:08

pdehaan reviewed Jun 12, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from f8d9ecd to 699d210 Compare June 12, 2015 23:41

pdehaan mentioned this pull request Jun 12, 2015

Run lint scripts on a prepush git hook #2592

Closed

shane-tomlinson reviewed Jun 14, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 699d210 to 651a676 Compare June 16, 2015 22:34

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 651a676 to c929dab Compare June 17, 2015 14:32

zaach reviewed Jun 17, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from c929dab to 1dc2a83 Compare June 17, 2015 18:21

zaach reviewed Jun 17, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 1dc2a83 to 6c3d639 Compare June 18, 2015 21:40

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 6c3d639 to 20afb6f Compare June 18, 2015 22:46

shane-tomlinson reviewed Jun 21, 2015
View reviewed changes

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 20afb6f to 55ccd71 Compare June 22, 2015 17:21

pdehaan added the avatars label Jun 28, 2015

pdehaan added the metrics label Jun 29, 2015

feat(avatars): add metrics for avatar cropping operations

8902331

eoger force-pushed the issue-2533-metrics-avatar-cropping branch from 55ccd71 to 8902331 Compare June 29, 2015 17:44

shane-tomlinson reviewed Jul 1, 2015
View reviewed changes

shane-tomlinson closed this Jul 1, 2015

shane-tomlinson removed the waffle:needs review label Jul 1, 2015

shane-tomlinson mentioned this pull request Jul 1, 2015

Add metrics for avatar cropping operations #2533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(avatars): add metrics for avatar cropping operations #2591

feat(avatars): add metrics for avatar cropping operations #2591

eoger commented Jun 12, 2015

pdehaan Jun 12, 2015

eoger Jun 12, 2015

pdehaan Jun 12, 2015

pdehaan commented Jun 12, 2015

pdehaan Jun 12, 2015

eoger commented Jun 12, 2015

pdehaan commented Jun 12, 2015

shane-tomlinson Jun 14, 2015

zaach Jun 16, 2015

shane-tomlinson Jun 16, 2015

zaach Jun 16, 2015

eoger commented Jun 16, 2015

shane-tomlinson commented Jun 17, 2015

eoger commented Jun 17, 2015

zaach Jun 17, 2015

eoger Jun 17, 2015

zaach Jun 17, 2015

eoger commented Jun 18, 2015

zaach commented Jun 18, 2015

eoger commented Jun 18, 2015

shane-tomlinson Jun 21, 2015

shane-tomlinson commented Jun 21, 2015

eoger commented Jun 22, 2015

vladikoff commented Jun 29, 2015

eoger commented Jun 29, 2015

shane-tomlinson Jul 1, 2015

shane-tomlinson commented Jul 1, 2015

feat(avatars): add metrics for avatar cropping operations #2591

feat(avatars): add metrics for avatar cropping operations #2591

Conversation

eoger commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan commented Jun 12, 2015

Choose a reason for hiding this comment

eoger commented Jun 12, 2015

pdehaan commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoger commented Jun 16, 2015

shane-tomlinson commented Jun 17, 2015

eoger commented Jun 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoger commented Jun 18, 2015

zaach commented Jun 18, 2015

eoger commented Jun 18, 2015

Choose a reason for hiding this comment

shane-tomlinson commented Jun 21, 2015

eoger commented Jun 22, 2015

vladikoff commented Jun 29, 2015

eoger commented Jun 29, 2015

Choose a reason for hiding this comment

shane-tomlinson commented Jul 1, 2015