feat(devices): add devices modal and additional metrics #4131
Conversation
|
Still WIP but you can play around with the fancy modals: |
| @@ -30,11 +31,12 @@ define(function (require, exports, module) { | |||
| * | |||
| * @param {object} inputEl - input element whose placeholder | |||
| * should be shown. | |||
| * @param {String} text - custom text for the floating label. | |||
shane-tomlinson
Sep 16, 2016
Member
Super pedantic nit, can you add [] around text since it's optional?
Super pedantic nit, can you add [] around text since it's optional?
| @@ -90,6 +92,18 @@ define(function (require, exports, module) { | |||
| this.focusFloatingPlaceholder($inputEl); | |||
| }, | |||
|
|
|||
| /** | |||
| * The ridiculous name is to avoid collisions with | |||
shane-tomlinson
Sep 16, 2016
Member
HA!
HA!
| floatingPlaceholderMixinOnSelect: function (event) { | ||
| var $inputEl = $(event.currentTarget); | ||
| this.showFloatingPlaceholder($inputEl, $inputEl.find('option:first').text()); | ||
| this.focusFloatingPlaceholder($inputEl); |
shane-tomlinson
Sep 16, 2016
Member
I realize focusFloatingPlaceholder was already there, but it seems misnamed because the placeholder element itself isn't being focused, only the styles are updated. I also can't think of a better name.
I realize focusFloatingPlaceholder was already there, but it seems misnamed because the placeholder element itself isn't being focused, only the styles are updated. I also can't think of a better name.
| @@ -20,8 +20,9 @@ define(function (require, exports, module) { | |||
| }, | |||
|
|
|||
| events: { | |||
| 'click .avatar-panel #back': BaseView.preventDefaultThen('_returnToAvatarChange'), | |||
| 'click .cancel': BaseView.preventDefaultThen('_closePanelReturnToSettings') | |||
| 'click .cancel': BaseView.preventDefaultThen('_closePanelReturnToSettings'), | |||
shane-tomlinson
Sep 16, 2016
Member
I don't see BaseView referenced anywhere else in this module. Maybe change the BaseView reference on line 13 to preventDefaultThen and use that reference here to make each line slightly more concise.
I don't see BaseView referenced anywhere else in this module. Maybe change the BaseView reference on line 13 to preventDefaultThen and use that reference here to make each line slightly more concise.
| @@ -45,6 +46,12 @@ define(function (require, exports, module) { | |||
| this.closePanel(); | |||
| }, | |||
|
|
|||
| _closePanelReturnToClients: function () { | |||
| this.navigate('settings/clients'); | |||
shane-tomlinson
Sep 16, 2016
•
Member
I understand why you did this, and makes sense, but I fear we are going to end up with a generic mixin with a lot of view specific routes and logic. I can easily imagine us adding a #back button to a non-avatar related view in the future and not understanding why clicking the button keeps going to settings/avatar/change. Can we generalize somehow, either by embedding the next screen view as an attribute on the button, or making the back buttons links whose href is set to the next step?
I understand why you did this, and makes sense, but I fear we are going to end up with a generic mixin with a lot of view specific routes and logic. I can easily imagine us adding a #back button to a non-avatar related view in the future and not understanding why clicking the button keeps going to settings/avatar/change. Can we generalize somehow, either by embedding the next screen view as an attribute on the button, or making the back buttons links whose href is set to the next step?
vladikoff
Sep 16, 2016
Author
Contributor
Yea we can do a lot of refactoring here. Follow up issue okay?
Yea we can do a lot of refactoring here. Follow up issue okay?
| this.reasonHelp = REASON_HELP[selectedValue]; | ||
| if (item.get('isCurrentDevice')) { | ||
| // if disconnected the current device then sign out | ||
| this._closePanelReturnToClients(); |
shane-tomlinson
Sep 16, 2016
Member
Calling _closePanelReturnToClients then navigateToSignIn will call this.navigate twice. What happens if you just call this.navigateToSignIn, that should close the panel, shouldn't it?
Calling _closePanelReturnToClients then navigateToSignIn will call this.navigate twice. What happens if you just call this.navigateToSignIn, that should close the panel, shouldn't it?
| this.toDisconnect = false; | ||
| this.reasonHelp = REASON_HELP[selectedValue]; | ||
| if (item.get('isCurrentDevice')) { | ||
| // if disconnected the current device then sign out |
shane-tomlinson
Sep 16, 2016
Member
If the user destroyed the current device, the user model will clear the signed in account and notify the browser. This comment should probably read if disconnected the current device, the user is automatically signed out.
If the user destroyed the current device, the user model will clear the signed in account and notify the browser. This comment should probably read if disconnected the current device, the user is automatically signed out.
vladikoff
Sep 16, 2016
Author
Contributor
👍
| * Closes the panel if device was disconnected. | ||
| */ | ||
| closePanelIfDisconnected () { | ||
| if (! this.toDisconnect) { |
shane-tomlinson
Sep 16, 2016
Member
I wonder if the logic would be more clear if toDisconnect was renamed to hasDisconnected and the boolean checks reversed?
I wonder if the logic would be more clear if toDisconnect was renamed to hasDisconnected and the boolean checks reversed?
vladikoff
Sep 16, 2016
Author
Contributor
👍
|
|
||
| /** | ||
| * Called on panel interaction. | ||
| * Closes the panel if device was disconnected. |
shane-tomlinson
Sep 16, 2016
Member
I'm a bit confused by how this works, does the user disconnect and then have to click on anything for the panel to close?
I'm a bit confused by how this works, does the user disconnect and then have to click on anything for the panel to close?
vladikoff
Sep 16, 2016
Author
Contributor
See https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R180
There are a few different scenarios
See https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R180
There are a few different scenarios
|
|
||
| return this.user.destroyAccountClient(this.user.getSignedInAccount(), item) | ||
| .then(() => { | ||
| // user has disconnect the device |
shane-tomlinson
Sep 16, 2016
Member
Once the client is destroyed, do you need to remove it from the clients collection passed in from the clients view?
Once the client is destroyed, do you need to remove it from the clients collection passed in from the clients view?
shane-tomlinson
Sep 16, 2016
Member
No, no you don't, I remembered the Clients collection listens for destroy events and does the right thing.
No, no you don't, I remembered the Clients collection listens for destroy events and does the right thing.
| const WindowMock = require('../../../mocks/window'); | ||
|
|
||
| describe('views/settings/client_disconnect', () => { | ||
| var metrics; |
shane-tomlinson
Sep 16, 2016
Member
You can use let on all these! Can you sort them too?
You can use let on all these! Can you sort them too?
|
|
||
| // test cancel | ||
| .then(click('.cancel-disconnect')) | ||
| .then(FunctionalHelpers.pollUntil(function () { |
shane-tomlinson
Sep 16, 2016
Member
It looks like you are waiting for client-disconnect to go away. I've always looked for a reason to use leadfoot's waitForDeletedByCssSelector. Valid use?
It looks like you are waiting for client-disconnect to go away. I've always looked for a reason to use leadfoot's waitForDeletedByCssSelector. Valid use?
shane-tomlinson
Sep 16, 2016
Member
Even if waitForDeletedByCssSelector doesn't work, I see this pattern repeated several times, it's probably worth creating a helper function in lib/helpers.js and using that.
Even if waitForDeletedByCssSelector doesn't work, I see this pattern repeated several times, it's probably worth creating a helper function in lib/helpers.js and using that.
vladikoff
Sep 16, 2016
Author
Contributor
+1 to add a helper, in the past waitForDeletedByCssSelector. did not work well, pollUntil makes more sense.
+1 to add a helper, in the past waitForDeletedByCssSelector. did not work well, pollUntil makes more sense.
| .then(click('.client:nth-child(2) .client-disconnect')) | ||
| .then(click('select.disconnect-reasons > option[value="lost"]')) | ||
|
|
||
| // wait until button is enabled |
shane-tomlinson
Sep 16, 2016
Member
is enabled or has gone away? Can waitForDeletedByCssSelector be used here too?
is enabled or has gone away? Can waitForDeletedByCssSelector be used here too?
vladikoff
Sep 19, 2016
Author
Contributor
enabled
enabled
|
|
||
| .then(click('#client-disconnect .primary')) | ||
|
|
||
| .then(click('#client-disconnect .reason-help')) |
shane-tomlinson
Sep 16, 2016
Member
What should clicking help do here?
What should clicking help do here?
vladikoff
Sep 16, 2016
Author
Contributor
https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R175
clicking on anything should close the modal
https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R175
clicking on anything should close the modal
| return this.navigate('settings/clients'); | ||
| } | ||
|
|
||
| this.item = clients.get(deviceId); |
shane-tomlinson
Sep 16, 2016
Member
Instead of this.item, would this.client be easier to follow?
Instead of this.item, would this.client be easier to follow?
| // receive the device collection and the item to delete | ||
| // if deleted the collection will be automatically updated in the settings panel. | ||
| let clients = this.model.get('clients'); | ||
| let deviceId = this.model.get('itemId'); |
shane-tomlinson
Sep 16, 2016
Member
Niether clients nor itemId are used past this point, would it be simpler to just pass in the expected client from clients.js?
Niether clients nor itemId are used past this point, would it be simpler to just pass in the expected client from clients.js?
shane-tomlinson
Sep 16, 2016
Member
If not, itemId could stand to be renamed to clientId.
If not, itemId could stand to be renamed to clientId.
vladikoff
Sep 19, 2016
Author
Contributor
"clientId" 👍
"clientId"
|
Overall this PR is pretty solid, nothing worrying. Most of my comments try to make the code more concise, or easier to understand, or just remove duplicate logic that has already been generalized. I have not yet tested this manually. |
| // if a device then ask for confirmation | ||
| if (clientType === Constants.CLIENT_TYPE_DEVICE) { | ||
| this.navigate('settings/clients/disconnect', { | ||
| clients: this._attachedClients, |
shane-tomlinson
Sep 16, 2016
Member
This seems to be where it'd be simpler to just pass in the client since neither clients nor the id are used for anything except fetching the client.
This seems to be where it'd be simpler to just pass in the client since neither clients nor the id are used for anything except fetching the client.
vladikoff
Sep 16, 2016
Author
Contributor
Yeah I had it working that away initially, but client must be destroyed from the clients collection. Otherwise it will be calling Backbone sync on destroy and XHR our: http://backbonejs.org/#Model-destroy and also this helps with: https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R199 ( After device removal has been confirmed, the dialog closes and, the row is overtaken by the area beneath it)
Yeah I had it working that away initially, but client must be destroyed from the clients collection. Otherwise it will be calling Backbone sync on destroy and XHR our: http://backbonejs.org/#Model-destroy and also this helps with: https://github.com/mozilla/fxa/pull/181/files#diff-9d765e5787806afc80bdaa86191886a8R199 ( After device removal has been confirmed, the dialog closes and, the row is overtaken by the area beneath it)
| * | ||
| * @param {Event} event | ||
| */ | ||
| selectOption (event) { |
shane-tomlinson
Sep 16, 2016
Member
Should trying to submit the form print a tooltip if no option has been selected?
Should trying to submit the form print a tooltip if no option has been selected?
vladikoff
Sep 16, 2016
Author
Contributor
We can refine that later if needed IMHO
We can refine that later if needed IMHO
| */ | ||
| selectOption (event) { | ||
| let optionIndex = this.$el.find(event.currentTarget).find(':selected').index(); | ||
| if (optionIndex === 0) { |
shane-tomlinson
Sep 16, 2016
Member
I think this function and it's attached event could possibly be removed by overriding isValidStart -
isValidStart () {
return this.$(':selected').index() > 0;
},
My theory is the change form event handler will call enableSubmitIfValid which will call isValidStart and do the right thing.
I think this function and it's attached event could possibly be removed by overriding isValidStart -
isValidStart () {
return this.$(':selected').index() > 0;
},My theory is the change form event handler will call enableSubmitIfValid which will call isValidStart and do the right thing.
|
Some more questions/comments. The "disconnect reason" screen is not keyboard accessible. Check out this video: Can the Once a disconnection is successful, if the user is shown "help text" it's not obvious how to close the screen. Keyboard users have no way of doing so. The second point on the help text, both include "You should change your Firefox Account password" but no easy way to do so is proviced. Should we link to Should a user who is currently going through signin confirmation be listed even though they have not yet synced? It's fun to play with on multiple browsers and see how quickly the user is disconnected! |
Maybe later after Phase 1 deploy
Just following the feature doc here. We can add keyboard close in this version though
After phase 1 probably
Post phase 1 but there is an issue mozilla/fxa#186 |
|
Looks really cool! Just a few minor things from me. |
| * @param {Event} event | ||
| */ | ||
| floatingPlaceholderMixinOnSelect: function (event) { | ||
| var $inputEl = $(event.currentTarget); |
vbudhram
Sep 16, 2016
Contributor
Should this be const? Noticed file uses var as well, but I think they could be updated to use const. File is not to big, so might be worthwhile to make consistent?
Should this be const? Noticed file uses var as well, but I think they could be updated to use const. File is not to big, so might be worthwhile to make consistent?
| text-align: left; | ||
| } | ||
|
|
||
| html[dir='rtl'] & { |
vbudhram
Sep 16, 2016
Contributor
👍
shane-tomlinson
Sep 19, 2016
Member
Was this a problem before with the old panels?
Was this a problem before with the old panels?
| it('has props', () => { | ||
| view.beforeRender(); | ||
| assert.ok(view.context().deviceName); | ||
| assert.ok(view.context().toDisconnect); |
vbudhram
Sep 16, 2016
Contributor
Looks like context returns reasonHelp as well, could add a check.
Looks like context returns reasonHelp as well, could add a check.
| return view.render().then(() => { | ||
| assert.ok(view.disableForm.calledOnce); | ||
| assert.notOk(view.enableForm.calledOnce); | ||
| assert.ok($(view.el).find('.primary.disabled').length, 'disabled button at first'); |
vbudhram
Sep 16, 2016
Contributor
nit: disabled button at first wording threw me off a little. Maybe some like submit button disabled or has disabled class?
nit: disabled button at first wording threw me off a little. Maybe some like submit button disabled or has disabled class?
| return view.submit().then(() => { | ||
| assert.notOk(view.toDisconnect); | ||
| assert.ok(view.render.calledOnce, 'not rendered, current device'); | ||
| assert.ok(TestHelpers.isEventLogged(metrics, 'settings.clients.disconnect.submit.suspicious')); |
vbudhram
Sep 16, 2016
•
Contributor
Mind updating the metrics docs with new ones?
Mind updating the metrics docs with new ones?
|
@vladikoff Don't forget the … after Disconnect… as specificed in the doc. Now needed as what follows is revocable. |
|
@vbudhram @shane-tomlinson final r? |
|
Travis not showing status but it is here: https://travis-ci.org/mozilla/fxa-content-server/pull_requests |
|
@vladikoff Just a couple notes. It was a little jarring when I disconnected only device and got signed out. Almost feel like there should be a prompt Otherwise, r+ from me! |
| <label class="label-helper"></label> | ||
| <div class="select-row"> | ||
| <select class="disconnect-reasons"> | ||
| <option value="reason">{{#t}}Reason for disconnecting...{{/t}}</option> |
Yea that's a good point. Maybe in the future we can tweak the messaging for 'current' device disconnect. |
|
This is in great shape! Only two changes requested:
Otherwise, looks great, tests well. Can you file follow-on issues for keyboard accessibility, styling of the select, possibly showing a close button on the "help text" to make it obvious what next steps are, and allow users to click on a "change password" link from the help text to make that path simpler? r+ with minor nits addressed. |
| <label class="label-helper"></label> | ||
| <div class="select-row"> | ||
| <select class="disconnect-reasons"> | ||
| <option value="reason">{{#t}}Reason for disconnecting...{{/t}}</option> |
| @@ -0,0 +1,40 @@ | |||
| <div id="client-disconnect"> | |||
| <section class="modal-panel"> | |||
| {{^hasDisconnected}} | |||
shane-tomlinson
Sep 19, 2016
Member
I like the hasDisconnected
I like the hasDisconnected
| @@ -26,7 +26,7 @@ | |||
| {{^isCurrentDevice}} | |||
| <div class="last-connected">{{lastAccessTimeFormatted}}</div> | |||
| {{/isCurrentDevice}} | |||
| <button class="settings-button warning client-disconnect" data-id="{{id}}" data-type="{{clientType}}">{{#t}}Disconnect{{/t}}</button> | |||
| <button class="settings-button warning client-disconnect" data-id="{{id}}" data-type="{{clientType}}">{{#t}}Disconnect…{{/t}}</button> | |||
shane-tomlinson
Sep 19, 2016
Member
It looks like here you've used the unicode ellipsis. Should this be …?
It looks like here you've used the unicode ellipsis. Should this be …?
| @@ -63,11 +65,11 @@ define(function (require, exports, module) { | |||
| } | |||
| }, | |||
|
|
|||
| focusFloatingPlaceholder: function (inputEl) { | |||
| focusLabelHelper: function (inputEl) { | |||
shane-tomlinson
Sep 19, 2016
Member
👍 - dig the name change.
|
|
||
| const REASON_SELECTOR = '.disconnect-reasons'; | ||
| const REASON_HELP = { | ||
| 'lost': t('We\'re sorry to hear about this. You should change your Firefox Account password, and look for ' + |
shane-tomlinson
Sep 19, 2016
Member
Thanks for checking.
Thanks for checking.
| * @returns {Boolean} | ||
| */ | ||
| isValidStart () { | ||
| return this.$(':selected').index() > 0; |
shane-tomlinson
Sep 19, 2016
Member
perfect!
perfect!
| * Closes the panel if device was disconnected. | ||
| */ | ||
| closePanelAfterDisconnect () { | ||
| if (this.hasDisconnected) { |
shane-tomlinson
Sep 19, 2016
Member
The inversion of variable semantics from this.needsDisconnect to this.hasDisconnected makes this much easier to read.
The inversion of variable semantics from this.needsDisconnect to this.hasDisconnected makes this much easier to read.
| text-align: left; | ||
| } | ||
|
|
||
| html[dir='rtl'] & { |
shane-tomlinson
Sep 19, 2016
Member
Was this a problem before with the old panels?
Was this a problem before with the old panels?
|
|
||
| #### settings/clients | ||
|
|
||
| * settings.clients.[clientType].disconnect - user is attempting to disconnect a client type |
shane-tomlinson
Sep 19, 2016
Member
Can you add the suffix's you have above - old, no, lost, suspicious?
Can you add the suffix's you have above - old, no, lost, suspicious?
vladikoff
Sep 19, 2016
Author
Contributor
Added
Added
| * @param {Number} [timeout] | ||
| * Timeout to wait until element is gone | ||
| */ | ||
| function pollUntilGoneByQSA(selector, timeout) { |
shane-tomlinson
Sep 19, 2016
Member
Huge 👍
Huge
( I cannot reply to that comment inline the review, sent GitHub a support request). Anyway, it was used for |







Fixes #4124