Closed Bug 1107255 Opened 5 years ago Closed 5 years ago

Clicking on a room, or starting a direct conversation should close the panel

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(firefox35 fixed, firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Iteration:
37.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: dmose, Mentored)

References

Details

(Whiteboard: [good first verify])

Attachments

(2 files, 2 obsolete files)

When clicking on a room, or starting a direct video/audio call, the panel should be closed automatically.

Other actions, e.g. blocking a contact, copying a room url, should keep the panel open.
Assignee: standard8 → dmose
backlog: --- → Fx35+
Priority: -- → P1
Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

This causes conversation entry URL clicks from the panel as well as audio and video call starts from the contact list to close the panel, but nothing else.

It uses the WindowCloseMixin that we paired on last week.  This patch is based on fx-team, and once it lands on mozilla-central, it probably wants to be uplifted.
Attachment #8533348 - Flags: review?(nperriault)
Comment on attachment 8533349 [details] [diff] [review]
Use WindowCloseMixin for existing, tested versions of window.close

This patch must be applied on top of the previous one, and converts all existing _tested_ calls (and their tests) to window.close to use the mixin. There are a bunch of untested calls which need to have tests written before they're converted.  I'm happy to file a spin-off bug to do that, if you wish.  I don't think this wants to be uplifted after it lands.
Attachment #8533349 - Flags: review?(nperriault)
Note that I'm happy (and would even prefer!) to do these as pair reviews, if you're inclined and available.
Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

Review of attachment 8533348 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=nperriault.

::: browser/components/loop/content/shared/js/mixins.js
@@ +69,5 @@
> +   * calling window.close() directly, use this mixin and call
> +   * this.closeWindow from your component.  In tests, use
> +   * loop.shared.mixins.setRootObject to substitute a fake window
> +   * beforeEach() and replace the real one for use by the framework
> +   * afterEach(.

Nit: afterEach()

Also, this last sentence would make a very good jsdoc for the whole module.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +34,5 @@
>      };
>  
> +    fakeWindow = {
> +      close: sandbox.stub(),
> +      //document: { addEventListener: function(){} }

Nit: should be deleted.

@@ +51,5 @@
> +
> +
> +  describe("ContactsList", function () {
> +    var listView;
> +    beforeEach(function() {

Nit: missing empty line above beforeEach.

@@ +56,5 @@
> +      navigator.mozLoop.calls = {
> +        startDirectCall: sandbox.stub(),
> +        clearCallInProgress: sandbox.stub()
> +      };
> +      navigator.mozLoop.contacts = {getAll: sandbox.stub()};

Nit: How about:

navigator.mozLoop = {
  calls: {…},
  contacts: {…}
};

@@ +62,5 @@
> +      listView = TestUtils.renderIntoDocument(loop.contacts.ContactsList());
> +    });
> +
> +    afterEach(function() {
> +      listView = null;

Nit: we should probably delete navigator.mozLoop here.
Attachment #8533348 - Flags: review?(nperriault) → review+
Comment on attachment 8533349 [details] [diff] [review]
Use WindowCloseMixin for existing, tested versions of window.close

Review of attachment 8533349 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=nperriault.

::: browser/components/loop/content/js/conversation.jsx
@@ +223,5 @@
>     * At the moment, it does more than that, these parts need refactoring out.
>     */
>    var IncomingConversationView = React.createClass({
> +    mixins: [sharedMixins.AudioMixin,
> +      sharedMixins.WindowCloseMixin],

Nit: Better indentation needed (make sure tis matches the style applied in similar situation).

::: browser/components/loop/content/js/conversationViews.jsx
@@ +193,5 @@
>     * Call failed view. Displayed when a call fails.
>     */
>    var CallFailedView = React.createClass({
> +    mixins: [Backbone.Events, sharedMixins.AudioMixin,
> +      sharedMixins.WindowCloseMixin],

Nit: indentation.
Attachment #8533349 - Flags: review?(nperriault) → review+
Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

Review of attachment 8533348 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/shared/js/mixins.js
@@ +69,5 @@
> +   * calling window.close() directly, use this mixin and call
> +   * this.closeWindow from your component.  In tests, use
> +   * loop.shared.mixins.setRootObject to substitute a fake window
> +   * beforeEach() and replace the real one for use by the framework
> +   * afterEach(.

Fixed.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +34,5 @@
>      };
>  
> +    fakeWindow = {
> +      close: sandbox.stub(),
> +      //document: { addEventListener: function(){} }

Done in the patch destined for nightly but not uplift.

@@ +51,5 @@
> +
> +
> +  describe("ContactsList", function () {
> +    var listView;
> +    beforeEach(function() {

Fixed.

@@ +56,5 @@
> +      navigator.mozLoop.calls = {
> +        startDirectCall: sandbox.stub(),
> +        clearCallInProgress: sandbox.stub()
> +      };
> +      navigator.mozLoop.contacts = {getAll: sandbox.stub()};

I thought I could do this while we were pairing, but I was wrong: navigator.mozLoop is actually defined in a higher scope in the tests.  I guess I could hoist or merge the objects, but that seems like more trouble than it's worth.

@@ +62,5 @@
> +      listView = TestUtils.renderIntoDocument(loop.contacts.ContactsList());
> +    });
> +
> +    afterEach(function() {
> +      listView = null;

Because the object itself is defined in an enclosing scope, the things that make sense to delete here are navigator.mozLoop.calls and navigator.mozLoop.contacts, so I've done that.
Comment on attachment 8533861 [details] [diff] [review]
Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]

With review comments addressed; carrying forward Niko's r+.
Attachment #8533861 - Flags: review+
Attachment #8533861 - Attachment description: Close panel on call conversation start → Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]
Comment on attachment 8533900 [details] [diff] [review]
[landed fx-team; should NOT uplift after it clears m-c] Fix tested Loop callers of window.close to use WindowCloseMixin, r=NiKo`

review comments fixed; carrying forward r+
Attachment #8533900 - Flags: review+
Attachment #8533900 - Attachment description: Fix tested Loop callers of window.close to use WindowCloseMixin, r=NiKo` → [landed fx-team; should NOT uplift after it clears m-c] Fix tested Loop callers of window.close to use WindowCloseMixin, r=NiKo`
https://hg.mozilla.org/mozilla-central/rev/13c5e6f5657d
https://hg.mozilla.org/mozilla-central/rev/bcc39a19cd51
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Dan -- Is this supposed to close the panel when the user clicks "start a conversation" (and creates a new room)?  Or does the panel close only when the user clicks on an existing room?
Flags: needinfo?(dmose)
Comment on attachment 8533861 [details] [diff] [review]
Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: UI annoyance (have to click to dismiss the panel after selecting a room)

[Describe test coverage new/current, TBPL]: on m-c for a few days; includes extensive tests (most of the patch)

[Risks and why]: low risk - just closes the panel on certain operations, includes extensive tests.

[String/UUID change made/needed]: none
Attachment #8533861 - Flags: approval-mozilla-beta?
Attachment #8533861 - Flags: approval-mozilla-aurora?
mreavy: only when the user clicks an existing conversation.  The reason for this is that "Start a conversation" only adds another entry to the list, it doesn't actually open the conversation window.

And now that I'm thinking about it a little harder, I suspect that most people, most of the time when they press "Start a Conversation" would be happier if the conversation window opened for them without requiring an extra click (in which case closing the panel then would make sense).  I'd suggest pinging Sevaan and/or filing a bug on that...
Flags: needinfo?(dmose)
Blocks: 1111011
Attachment #8533861 - Flags: approval-mozilla-beta?
Attachment #8533861 - Flags: approval-mozilla-beta+
Attachment #8533861 - Flags: approval-mozilla-aurora?
Attachment #8533861 - Flags: approval-mozilla-aurora+
Mentor: anthony.s.hughes
Flags: qe-verify-
Whiteboard: [good first verify]
Iteration: --- → 37.1
Blocks: 1118402
You need to log in before you can comment on or make changes to this bug.