1171940 - Move the context views to the chat area inside conversations

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

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.

Comment 1

[Mark Banner (:standard8)]

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.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: move the context display into the chat area

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: remove the mode-switching and have only one-mode EditContext view

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: introduce an 'Edit Context' button to the conversation toolbar

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

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: move the context display into the chat area

Carrying over r=Standard8.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: remove the mode-switching and have only one-mode EditContext view

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: introduce an 'Edit Context' button to the conversation toolbar

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: update/ fix tests that my patches have broken

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

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: remove the mode-switching and have only one-mode EditContext view

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: update/ fix tests that my patches have broken

This fixes _A LOT_ of warnings inside tests.

Comment 12

[Mike de Boer [:mikedeboer]]

(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 14

[Mark Banner (:standard8)]

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

Comment 15

[Mark Banner (:standard8)]

(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 16

[Mark Banner (:standard8)]

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 17

[Mark Banner (:standard8)]

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?

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: remove the mode-switching and have only one-mode EditContext view

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.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.2: introduce an 'Edit Context' button to the conversation toolbar

Carrying over r=Standard8

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.2: update/ fix tests that my patches have broken

Carrying over r=Standard8 (addressed review comments)

Comment 22

[Mike de Boer [:mikedeboer]]

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

Comment 23

[Mark Banner (:standard8)]

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.

Comment 24

[Sevaan Franks [:sevaan]]

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

Comment 25

[Mike de Boer [:mikedeboer]]

(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! ;-)

Comment 26

[Sevaan Franks [:sevaan]]

(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

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.4: remove the mode-switching and have only one-mode EditContext view

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

Comment 28

[Mark Banner (:standard8)]

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 29

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5: new tests

Comment 30

[Mark Banner (:standard8)]

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"

Comment 31

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.1: new tests

I'm glad you mentioned that!

Comment 32

[Pulsebot]

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

Comment 33

[Phil Ringnalda (:philor)]

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 34

[Mike de Boer [:mikedeboer]]

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 35

[Mike de Boer [:mikedeboer]]

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 36

[Mike de Boer [:mikedeboer]]

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 37

[Mike de Boer [:mikedeboer]]

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 38

[Mike de Boer [:mikedeboer]]

Comment on attachment 8629352 [details] [diff] [review]
Patch 5.1: new tests

Please see comment 34 for the approval comment of this patch.

Comment 39

[Ada [:adalucinet]]

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 40

[Kate Glazko]

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 41

[Kate Glazko]

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 42

[Kate Glazko]

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 43

[Kate Glazko]

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 44

[Kate Glazko]

Comment on attachment 8629352 [details] [diff] [review]
Patch 5.1: new tests

Approving uplift to Aurora because verified in m-c, tests pass.

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 48

[Mike de Boer [:mikedeboer]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47)
> Needs rebasing for Aurora uplift.

I was already working on it ;-) Thanks!

Comment 49

[Mike de Boer [:mikedeboer]]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Target milestone tracks trunk landing.