Closed Bug 1033843 Opened 5 years ago Closed 5 years ago

land Reactified version of conversation window

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: dmose, Unassigned)

References

Details

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

Attachments

(2 files, 1 obsolete file)

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 :)
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
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!
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)
Attached image 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]
0001-Bug-1033843-Reactified-conversation-view.patch

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+
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b342d60245
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/c1b342d60245
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Marking this verified fixed based on current UI in Firefox 33 and 34.
Status: RESOLVED → VERIFIED
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.