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.

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

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 on attachment 8666738 [details] [diff] [review] Leave / Exit conversation button always present Added hide hangup button functionality and removed unused property strings.

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?

> * 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.

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?

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

(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

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.

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

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 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 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.

Need to integrate upstream changes

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.

bug: 1211024 added for tool-tip default styling.

Rebase against fx-team

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 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 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 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 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.

Minor review comments addressed.

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