Once text chat is enabled by default, we should move the context view in _both_ the desktop and standalone clients to reside inside the text chat area. It will look like a pimped up chat message. See the linked URL to see how this element should be styled.

Mike: Note, I've designed the text chat view to make it easy to add the context to it for desktop. Search in textChatView.jsx for the XXX for this bug - that needs changing to just exclude the room name content type.

Next up is to find, fix & update all the broken unit tests and the ui-showcase.

Comment on attachment 8627167 [details] [diff] [review] Patch 1: move the context display into the chat area Review of attachment 8627167 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, though you forgot to re-generate the js files. r=Standard8 modulo the tests being fixed and the new ones added as necessary. ::: browser/components/loop/content/shared/js/textChatStore.js @@ +104,5 @@ > + var isContext = message.contentType === CHAT_CONTENT_TYPES.CONTEXT; > + if (isContext) { > + var contextUpdated = false; > + for (var i = 0, l = newList.length; i < l; ++i) { > + // Replace the current context message with the provided update. I'll mention it here so it isn't forgotten - this needs a new test for the change you're making.

Carrying over r=Standard8.

My next patch will contain new unit tests for the updated flows and context views behavior and edit context button.

This fixes _A LOT_ of warnings inside tests.

(In reply to Mike de Boer [:mikedeboer] from comment #11) > This fixes _A LOT_ of warnings inside tests. ...apart from fixing a few not so obvious bugs.

Comment on attachment 8627640 [details] [diff] [review] Patch 2.2: remove the mode-switching and have only one-mode EditContext view Review of attachment 8627640 [details] [diff] [review]: ----------------------------------------------------------------- This isn't far off, but I'd really like to see the simpler state management for showEditContext ::: browser/components/loop/content/js/roomViews.jsx @@ +171,5 @@ > }, > > + componentWillReceiveProps: function(nextProps) { > + if (this.props.showEditContext !== nextProps.showEditContext) { > + this.setState({ showEditContext: nextProps.showEditContext }); I think we should drop the local state tracking of the showEditContext. It might mean exposing add context click up the tree a bit more, but I'm thinking that's likely to go away soon, and having state management for the same thing in two different places is a little strange. @@ -570,3 @@ > <button className="room-context-btn-close" > onClick={this.handleCloseClick} > - title={mozL10n.get("context_hide_tooltip")}/> I believe context_hide_tooltip/context_edit_tooltip can be removed from loop.properties now. ::: browser/components/loop/content/shared/css/conversation.css @@ -970,1 @@ > .room-context-content { Should be able to remove this class now. I think we can also simplify the .room-context.editMode rules (since its always editMode now).

(In reply to Mark Banner (:standard8) from comment #14) > @@ -570,3 @@ > > <button className="room-context-btn-close" > > onClick={this.handleCloseClick} > > - title={mozL10n.get("context_hide_tooltip")}/> > > I believe context_hide_tooltip/context_edit_tooltip can be removed from > loop.properties now. Ok, so context_edit_tooltip is used in part 3. Still leaves the context_hide_tooltip to be removed though.

Comment on attachment 8627197 [details] [diff] [review] Patch 3.1: introduce an 'Edit Context' button to the conversation toolbar Review of attachment 8627197 [details] [diff] [review]: ----------------------------------------------------------------- I think this is reasonable but please get UX-review before landing. The main bits that seem strange to me are: - The Invite view overlays the context display. This means you can't read nor access the full context without someone else in the room or editing it. - Editing context now takes place at full height. This seems a little weird that there's a lot of blank space. Maybe if it was adjusted so the existing context can still be seen then it would be more appropriate.

Comment on attachment 8627641 [details] [diff] [review] Patch 4.1: update/ fix tests that my patches have broken Review of attachment 8627641 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r+ assuming you fix the total warnings in the ui-showcase, and assuming there's another patch coming for test additions (I think you said there was). TEST-UNEXPECTED-FAIL | index.html | Unexpected number of warnings detected in UI-Showcase - Got: 29 Got: 29 Expected: 53 ::: browser/components/loop/content/js/contacts.jsx @@ +126,5 @@ > {mozL10n.get("gravatars_promo_message_learnmore")} > </a> > ) > }); > + console.log("captions", mozL10n.get("gravatars_promo_button_nothanks"), Left-in debug?

Thanks for the suggestions! I applied them, but didn't remove the strings, because I use them still in Patch no. 3 (both). However, they might need some tweaking, pending ui-r.

Carrying over r=Standard8

Carrying over r=Standard8 (addressed review comments)

Sevaan, can you watch this video of about 3 minutes - https://vimeo.com/132418735 - and tell me what you think?

Comment on attachment 8628184 [details] [diff] [review] Patch 2.3: remove the mode-switching and have only one-mode EditContext view Review of attachment 8628184 [details] [diff] [review]: ----------------------------------------------------------------- That's better. ::: browser/components/loop/content/js/roomViews.jsx @@ +206,5 @@ > > handleAddContextClick: function(event) { > event.preventDefault(); > > + if (this.props.onAddContextClick) { According to eslint, this needs adding to propTypes. @@ +716,5 @@ > dispatcher={this.props.dispatcher} > error={this.state.error} > mozLoop={this.props.mozLoop} > + onEditContextClose={this.handleEditContextClose} > + onAddContextClick={this.handleAddContextClick} eslint says these should be sorted alphabetically.

(In reply to Mike de Boer [:mikedeboer] from comment #22) > Sevaan, can you watch this video of about 3 minutes - > https://vimeo.com/132418735 - and tell me what you think? Hey Mike, Thanks for putting this together. A few things jump out at me: - The context shouldn't be behind the overlay. Only the video area should be beneath that. - The invite buttons shouldn't be covering the context. They should be over the video window. - Where did the decision come from to add the edit button to the toolbar at the top? Was that something I provided (there have been a lot of versions)? Not sure that's the best place for it. This is a minor concern though since the layout is changing for the refresh. - Add some padding above the "Let's Talk About" so it's not right at the top of that container. - When you submitted some chats and then went to edit the context, the chats were above the overlay while nothing else was. - The Time stamp in chat should be on the far-left for my bubbles and the far-right for text submitted by the other person. - The edit conversation form during a call should have a solid colour background, or the gradient should be much darker as the video distracts from the form.

(In reply to Sevaan Franks [:sevaan] from comment #24) > - The context shouldn't be behind the overlay. Only the video area should be > beneath that. Alright! Makes sense to me :) > - The invite buttons shouldn't be covering the context. They should be over > the video window. We'll get that for free when I implement the above. > - Where did the decision come from to add the edit button to the toolbar at > the top? Was that something I provided (there have been a lot of versions)? Yes, because the context bubble doesn't have an obvious place to add an 'Edit Context' button, whereas right now it's the little pencil icon on the interstitial. So that's when you requested to put the toolbar button there to at least keep the 'editability' around inside the conversation window. > - When you submitted some chats and then went to edit the context, the chats > were above the overlay while nothing else was. I noticed that too... weird drawing 'glitch', but that will be fixed when I moved the overlay to only be above the remote video. > - The Time stamp in chat should be on the far-left for my bubbles and the > far-right for text submitted by the other person. Can you re-check the design elements that Vicky posted? I'll probably defer this work to a follow-up bug, but we'll see. > - The edit conversation form during a call should have a solid colour > background, or the gradient should be much darker as the video distracts > from the form. I will make it mucho darker. Thanks for watching! ;-)

(In reply to Mike de Boer [:mikedeboer] from comment #25) > > - The Time stamp in chat should be on the far-left for my bubbles and the > > far-right for text submitted by the other person. > > Can you re-check the design elements that Vicky posted? I'll probably defer > this work to a follow-up bug, but we'll see. Definitely aligned to the edges of the chat, and not to the bubbles. http://i.sevaan.com/image/0m283y1p1U02

Requesting re-review for this change, as it moves the invitation overlay inside the media elements container.

Comment on attachment 8629285 [details] [diff] [review] Patch 2.4: remove the mode-switching and have only one-mode EditContext view Looks good. Might get in the way of some things I'm working on, but we'll see ;-)

Comment on attachment 8629339 [details] [diff] [review] Patch 5: new tests Review of attachment 8629339 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but I think we should get a test for the textChatStore.js changes from patch 1.1 as well. ::: browser/components/loop/test/desktop-local/roomViews_test.js @@ +256,5 @@ > + expect(view.getDOMNode().querySelector(".room-invitation-addcontext")). > + to.not.eql(null); > + }); > + > + it("should show the edit context view when the link is clicked", function() { I would suggest renaming this to "should call a callback when the link is clicked"

I'm glad you mentioned that!

https://hg.mozilla.org/integration/fx-team/rev/f338aab869d3 https://hg.mozilla.org/integration/fx-team/rev/d34865c942e3 https://hg.mozilla.org/integration/fx-team/rev/4ea7fbda92cc https://hg.mozilla.org/integration/fx-team/rev/cbe3ff5e3877 https://hg.mozilla.org/integration/fx-team/rev/ea4206694632

https://hg.mozilla.org/mozilla-central/rev/f338aab869d3 https://hg.mozilla.org/mozilla-central/rev/d34865c942e3 https://hg.mozilla.org/mozilla-central/rev/4ea7fbda92cc https://hg.mozilla.org/mozilla-central/rev/cbe3ff5e3877 https://hg.mozilla.org/mozilla-central/rev/ea4206694632

Comment on attachment 8627195 [details] [diff] [review] Patch 1.1: move the context display into the chat area Approval Request Comment [Feature/regressing bug #]: Loop/ Hello text chat feature [User impact if declined]: Text chat is riding the trains for Fx 41, but without this visual change the conversation window with context data attached will look weird. As always with our patches, bit less than half of the patch size is code generated by React. [Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass. [Risks and why]: minor, things will look better in fact. [String/UUID change made/needed]: n/a.

Comment on attachment 8628185 [details] [diff] [review] Patch 3.2: introduce an 'Edit Context' button to the conversation toolbar Please see comment 34 for the approval comment of this patch.

Comment on attachment 8628186 [details] [diff] [review] Patch 4.2: update/ fix tests that my patches have broken Please see comment 34 for the approval comment of this patch.

Comment on attachment 8629285 [details] [diff] [review] Patch 2.4: remove the mode-switching and have only one-mode EditContext view Please see comment 34 for the approval comment of this patch.

Comment on attachment 8629352 [details] [diff] [review] Patch 5.1: new tests Please see comment 34 for the approval comment of this patch.

Verified fixed on 42.0a1 (from 2015-07-08), across platforms [1]. Although, on Ubuntu 12.04 and 14.04, both 32-bit, found bug 1182055. Marking as verified since the remaining issue is tracked separately. [1] Windows 7 64-bit, Windows 10 64-bit and Mac OS X 10.9.5

Comment on attachment 8627195 [details] [diff] [review] Patch 1.1: move the context display into the chat area Approving uplift to Aurora because verified in m-c, tests pass.

Comment on attachment 8628185 [details] [diff] [review] Patch 3.2: introduce an 'Edit Context' button to the conversation toolbar Approving uplift to Aurora because verified in m-c, tests pass.

Comment on attachment 8628186 [details] [diff] [review] Patch 4.2: update/ fix tests that my patches have broken Approving uplift to Aurora because verified in m-c, tests pass.

Comment on attachment 8629285 [details] [diff] [review] Patch 2.4: remove the mode-switching and have only one-mode EditContext view Approving uplift to Aurora because verified in m-c, tests pass.

Comment on attachment 8629352 [details] [diff] [review] Patch 5.1: new tests Approving uplift to Aurora because verified in m-c, tests pass.

Ryan, have we landed these patches that were approved a few days back into m-a? I don't see a link and therefore wondering. Thanks!

Ugh, no we didn't. It got missed by one of the bug queries because the Target Milestone wasn't set and by the other because the firefox41 tracking flags weren't set to affected. Fail all around. Anyway, I'm setting the flags now which should get it back on the radar.

Needs rebasing for Aurora uplift.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47) > Needs rebasing for Aurora uplift. I was already working on it ;-) Thanks!

Pushed to m-a as: https://hg.mozilla.org/releases/mozilla-aurora/rev/692c3862907b https://hg.mozilla.org/releases/mozilla-aurora/rev/6b82550b6386 https://hg.mozilla.org/releases/mozilla-aurora/rev/bed330496c8b https://hg.mozilla.org/releases/mozilla-aurora/rev/0c8e3181f2f6 https://hg.mozilla.org/releases/mozilla-aurora/rev/b411cef27c22

Target milestone tracks trunk landing.