1201308 - Leave / Exit conversation button always present

Status: RESOLVED FIXED

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

Description

[Andrei Oprea [:andreio]]

Attachment: Loop standalone. Leave button should not be visible after closing call.

STR: Open a conversation panel. Exit / Leave button has text overlay.

STR: Open standalone Loop and start a call. Hang up. Exit / Leave button is still present.

Expected result: Button should not have text overlay. Button should not be present after leaving the standalone conversation.

See screenshot for more information.

Comment 1

[Andrei Oprea [:andreio]]

Attachment: Conversation view. Leave / Exit button should not have text overlay.

Comment 2

[:shell escalante]

mark will check it out - not seeing in current stuff but will investigate.

Comment 3

[Andrei Oprea [:andreio]]

Overlay was fixed in latest nightly. Discussion on IRC concluded that we should hide the button http://logs.glob.uno/?c=mozilla%23loop#c46652

Comment 4

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 5

[Chris Rafuse :crafuse]

Comment on attachment 8666738 [details] [diff] [review]
Leave / Exit conversation button always present

Added hide hangup button functionality and removed unused property strings.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8666738 [details] [diff] [review]
Leave / Exit conversation button always present

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

Two questions for Sevaan:

* Do we want to keep the tooltip for the hangup button?
* Right now, the standalone hangup button is displayed disabled before someone joins the room, since there's not yet anything to hang up.  Should we just hide the button in that case?

Comment 7

[Sevaan Franks [:sevaan]]

> * Do we want to keep the tooltip for the hangup button?

Yes, but let's make it "Leave" rather than "Hang up"

> * Right now, the standalone hangup button is displayed disabled before
> someone joins the room, since there's not yet anything to hang up.  Should
> we just hide the button in that case?

Yes please.

Comment 8

[Chris Rafuse :crafuse]

Attachment: Convo Invite Panel Tooltip

Comment 9

[Chris Rafuse :crafuse]

Attachment: Convo Popout Panel Tooltip

Comment 10

[Chris Rafuse :crafuse]

Attachment: Convo Room Window Tooltip

Comment 11

[Sevaan Franks [:sevaan]]

Chris, are those the default look for tooltips in Linux? Or have they been styled black like that.

Also, is the positioning just system/browser default?

Comment 12

[Chris Rafuse :crafuse]

Sevaan: They are default title attributes (Linux default I would guess) and reused the string that had the correct wording already.

Comment 13

[Chris Rafuse :crafuse]

(In reply to Sevaan Franks [:sevaan] from comment #11)
> Chris, are those the default look for tooltips in Linux? Or have they been
> styled black like that.
> 
> Also, is the positioning just system/browser default?

Probably Linux system browser defaults.
Styling possible in CSS3:
http://stackoverflow.com/questions/2011142/how-to-change-the-style-of-title-attribute-inside-the-anchor-tag

Comment 14

[Sevaan Franks [:sevaan]]

Just the default styling used on other tooltips in the browser is what we should be using for our tooltips.

If you can confirm that the tooltips reflect the styling of their respective systems (Windows, Mac), then these are +r'd for the string.

Comment 15

[Chris Rafuse :crafuse]

Attachment: Windows Tooltip

This shows what the Windows version looks like in nightly. Not sure why it is a regular Leave button.

Comment 16

[Sevaan Franks [:sevaan]]

Comment on attachment 8667513 [details]
Windows Tooltip

Well, obviously in this case the tooltip should say Leave to match the text in the button.

It should also be an icon, but it's all being refreshed soon, so it's not a high-priority to do this, if at all at this stage.

Comment 17

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 18

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 19

[Chris Rafuse :crafuse]

Comment on attachment 8668733 [details] [diff] [review]
Leave / Exit conversation button always present

Added correct string for Leave button.  Removed unneeded tests for Hangup label and disable button features.

Comment 20

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 21

[Chris Rafuse :crafuse]

Comment on attachment 8668735 [details] [diff] [review]
Leave / Exit conversation button always present

Added correct string for Leave button.  Removed unneeded tests for Hangup label and disable button features.

Comment 22

[Chris Rafuse :crafuse]

Need to integrate upstream changes

Comment 23

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 24

[Chris Rafuse :crafuse]

Comment on attachment 8669200 [details] [diff] [review]
Leave / Exit conversation button always present

Merged with up stream changes.  Created hangup control button module and display hide if not in conversation.

Comment 25

[Chris Rafuse :crafuse]

bug: 1211024 added for tool-tip default styling.

Comment 26

[Chris Rafuse :crafuse]

Attachment: convo Panel

Comment 27

[Chris Rafuse :crafuse]

Attachment: standalone Leave Button

Comment 28

[Chris Rafuse :crafuse]

Attachment: standalone With Tooltip

Comment 29

[Chris Rafuse :crafuse]

Attachment: Convo Window leave button

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Leave/Exit button should not be visible after closing call

Rebase against fx-team

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8669200 [details] [diff] [review]
Leave / Exit conversation button always present

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

Thanks for the patch, and sorry for the delay.  Here's a rebased version with some comments...

::: browser/components/loop/content/shared/js/views.jsx
@@ +47,5 @@
> +      return cx(classesObj);
> +    },
> +
> +    _getTitle: function() {
> +      //console.log("this.props.title", this.props.title);

Please remove this.

@@ +534,5 @@
>          "conversation-toolbar": true,
>          "idle": this.state.idle
>        });
> +
> +      var hideButtons = (!this.props.video.visible && !this.props.audio.visible);

If this is framed in the positive, it's easier to read:

(this.props.video.visible || this.props.audio.visible)

Separately from that, are we sure that we're not losing any behavior that we need by not checking the room state at all?

@@ +549,5 @@
>              <li className="conversation-toolbar-btn-box btn-hangup-entry">
> +                <HangUpControlButton action={this.handleClickHangup}
> +                                     title={mozL10n.get("rooms_leave_button_label")}
> +                                     visible={!hideButtons}/>
> +            </ li > : null

I think the visible property is not needed at all.  Can't you just change the conditional to "this.props.showHangup && !hidebuttons"?

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +915,5 @@
> +        //  function() {
> +        //    activeRoomStore.setStoreState({roomState: ROOM_STATES.HAS_PARTICIPANTS});
> +        //
> +        //    expect(getLeaveButton(view).disabled).eql(false);
> +        //  });

I think these tests want to go away (i.e. not just be commented out), since we're no longer predicating the display of the hangup button on room state.

However, we do want to replace them with some tests that exercise how the new behavior works (i.e. tests this.props.*.visible).

Comment 32

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 33

[Chris Rafuse :crafuse]

Comment on attachment 8671428 [details] [diff] [review]
Leave / Exit conversation button always present

Cleaned up as per comments.  Modified tests for visibility feature. Combined variables for visibility conditional test.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8671428 [details] [diff] [review]
Leave / Exit conversation button always present

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

As discussed, please remove the hide-yourself semantic, and fix the menu issue also.

Comment 35

[Chris Rafuse :crafuse]

Attachment: Leave / Exit conversation button always present

Comment 36

[Chris Rafuse :crafuse]

Comment on attachment 8672035 [details] [diff] [review]
Leave / Exit conversation button always present

now remove the toolbar Line item if buttons are not shown.
changed showButtons condition to an OR statement. 
removed console.logs re-tested and removed unnecessary method from button component.

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8672035 [details] [diff] [review]
Leave / Exit conversation button always present

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

OK, looks good, thanks for driving this forward!  I've commented on some minor bits.  Rather than doing another iteration here, I'll just make these changes and land.  r=dmose

::: browser/components/loop/content/shared/js/views.jsx
@@ +18,5 @@
> +   * Required props:
> +   * - {Function} action  Function to be executed on click.
> +   * - {String}   title   Tooltip functionality.
> +   */
> +  var HangUpControlButton = React.createClass({

We can add a PureRenderMixin to possibly increase performance, since the render for this class will always produce the same thing from identical props and state.

@@ +41,5 @@
> +    },
> +
> +    render: function() {
> +      return (
> +          <button className={this._getClasses()}

Since the class names are no longer dynamic, I think it makes things more readable just to use a string literal here and do away with _getClasses.

@@ +520,5 @@
>        var conversationToolbarCssClasses = cx({
>          "conversation-toolbar": true,
>          "idle": this.state.idle
>        });
> +      var showButtons = (this.props.video.visible || this.props.audio.visible);

Unnecessary surrounding parens here.

@@ +532,5 @@
> +          this.props.showHangup && showButtons ?
> +          <li className="conversation-toolbar-btn-box btn-hangup-entry">
> +            <HangUpControlButton action={this.handleClickHangup}
> +                                 title={mozL10n.get("rooms_leave_button_label")}/>
> +          </ li > : null

Nit: unnecessary spaces.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Leave / Exit conversation button always present

Minor review comments addressed.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1b26ac6f564f

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Pushed with off-by-one indentation errors in tests fixed.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b26ac6f564f