bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

land Reactified version of conversation window

VERIFIED FIXED in mozilla33


Hello (Loop)
4 years ago
4 years ago


(Reporter: dmose, Unassigned)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [p=2][qa+])


(2 attachments, 1 obsolete attachment)



4 years ago
This should just involve get a patch split out and reviewed.  That said, there are several things that we want to be sure work:

* in-browser window
* standalone window
* (in both of the above), that l10n falls back to english if there is no translation for the locale of the browser
Whiteboard: [p=2]
I've started work on ConversationToolbar component and porting/fixing tests for the rest of the components.
You can follow along on the progress in my github [0] and branch off of that if it has the right approach.

[0] https://github.com/piatra/gecko-dev/compare/dmose:react-conversation-bug-1033843...react-conversation-bug-1033843
Assignee: nobody → nperriault
Update: current state of work is viewable here https://github.com/dmose/gecko-dev/compare/react-conversation-bug-1033843

Tests are yet to be fully ported, though good progress has been made with the views, and the app is working :)

Comment 3

4 years ago
Niko wrote:

> For the view code, not much to say, the code should be self-explanatory (well, I
> hope). It could probably be factored a bit more (eg. I’m wondering for media video > buttons, lots of duplicated boilerplate).

I spent some time (not as much as I would have liked, but...), starting in on the tests.  It became apparent pretty quickly that testing the code well as-written was 
going to be painful: see this gist [0].

I could either copy-paste lots of the button state tests, as I first attempted (note that the first test and the commented out test are 95% dups, leaving something fragile and error prone.

Or, I could do the meta-programming exemplified by the forEach thing, which is painful to read, to the point where it's hard to convince myself that it's testing the right stuff, even though I'm pretty sure it is.

My gut is that this effectively means that we're going to be forced to factor our code apart into the simplest possible building blocks, because that is likely to be the only way to write maintainable tests.  Ultimately a good thing, I suspect, but it means the code I wrote should probably be thrown away, and the buttons factored out and tested separately.

[0] https://gist.github.com/anonymous/c819a4387d639d2cee30

Comment 4

4 years ago
I forgot to mention that the combinatorics get exciting in part because those are just the audio mute tests, and the video mute tests are 95% identical to them!
Created attachment 8454444 [details] [diff] [review]

Note: I've not ported the streaming media icons stuff because it's been discarded in bug 1000152 (https://bugzilla.mozilla.org/show_bug.cgi?id=1000152#c37)

I could also update wording for mute button titles, though it would probably help to have the final one to use first, hence putting :darrin in cc :)
Attachment #8454444 - Flags: review?(standard8)
Attachment #8454444 - Flags: feedback?(dhenein)
Created attachment 8455289 [details]
Mute Buttons Video

Darrin, I've done a recording of the mute buttons (ignore the colour quality), can you sign off on the mute toolbar indications & hovers etc - Nicolas has implemented what was decided following the previous bug.
Attachment #8455289 - Flags: ui-review?(dhenein)
Comment on attachment 8454444 [details] [diff] [review]

Review of attachment 8454444 [details] [diff] [review]:

Assuming that Darrin signs-off on the UX, then this has r=Standard8 with the comments addressed.

::: browser/components/loop/content/shared/css/conversation.css
@@ +76,2 @@
>  .conversation .controls .btn-mute-audio.streaming {
>    background-image: url(../img/audio-highlight-14x14.png);

Looks like this is no longer needed.

You also need to hg rm the removed images.

::: browser/components/loop/content/shared/js/views.jsx
@@ +120,5 @@
> +      classesObj["btn-mute-" + this.props.type] = true;
> +      // title
> +      var title = __(this.props.enabled ?
> +                     "mute_local_" + this.props.type + "_button" :
> +                     "unmute_local_" + this.props.type + "_button");

There's UX showing muting of remote video, so I think we could just change type to 'local_{video,audio}' now, and there's less to do later.

@@ +197,5 @@
> +
> +    getInitialState: function() {
> +      return {
> +        video: {enabled: false},
> +        audio: {enabled: false}

I think it might be worth a bit of explanation in a comment that we're using "enabled" actually means/implies "muted". Its a bit confusing with the different levels of views here, and it took me a while to work out that "enabled" does actually mean the same thing in all three of the views.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +18,4 @@
>  incoming_call=Incoming call
>  accept_button=Accept
>  decline_button=Decline
> +hangup_button=Hangup

I think for all of these, I'd prefer to go with *_button_title to be slightly more explicit (unfortunately we can't keep the *_button.title)
Attachment #8454444 - Flags: review?(standard8) → review+
Attachment #8455289 - Attachment is patch: false
Attachment #8455289 - Attachment mime type: text/plain → image/gif
Comment on attachment 8455289 [details]
Mute Buttons Video

Looks awesome :)
Attachment #8455289 - Flags: ui-review?(dhenein) → ui-review+
Created attachment 8455402 [details] [diff] [review]

Addressed review comments.
Attachment #8454444 - Attachment is obsolete: true
Attachment #8454444 - Flags: feedback?(dhenein)
Attachment #8455402 - Flags: review?(standard8)
Attachment #8455402 - Flags: review?(standard8) → review+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
Last Resolved: 4 years ago
Resolution: --- → FIXED
Marking this verified fixed based on current UI in Firefox 33 and 34.
QA Contact: anthony.s.hughes
Whiteboard: [p=2] → [p=2][qa+]
You need to log in before you can comment on or make changes to this bug.