Closed Bug 1171940 Opened 4 years ago Closed 4 years ago

Move the context views to the chat area inside conversations

Categories

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

defect
Points:
8

Tracking

(firefox41 fixed, firefox42 verified)

VERIFIED FIXED
mozilla42
Iteration:
42.1 - Jul 13
Tracking Status
firefox41 --- fixed
firefox42 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [context][chat])

Attachments

(5 files, 10 obsolete files)

25.43 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
24.03 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
61.82 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
52.30 KB, patch
standard8
: review+
Details | Diff | Splinter Review
6.65 KB, patch
standard8
: review+
Details | Diff | Splinter Review
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.
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 12
Whiteboard: [context][chat]
Depends on: 1174714
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 41.3 - Jun 29
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.
No longer blocks: 1084991
Attachment #8627167 - Flags: review?(standard8)
Next up is to find, fix & update all the broken unit tests and the ui-showcase.
Attachment #8627169 - Flags: review?(standard8)
Points: 5 → 13
Points: 13 → 8
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.
Attachment #8627167 - Flags: review?(standard8) → review+
Carrying over r=Standard8.
Attachment #8627167 - Attachment is obsolete: true
Attachment #8627195 - Flags: review+
Attachment #8627168 - Attachment is obsolete: true
Attachment #8627168 - Flags: review?(standard8)
Attachment #8627196 - Flags: review?(standard8)
Attachment #8627169 - Attachment is obsolete: true
Attachment #8627169 - Flags: review?(standard8)
Attachment #8627197 - Flags: review?(standard8)
My next patch will contain new unit tests for the updated flows and context views behavior and edit context button.
Attachment #8627199 - Flags: review?(standard8)
Blocks: 1178379
Attachment #8627196 - Attachment is obsolete: true
Attachment #8627196 - Flags: review?(standard8)
Attachment #8627640 - Flags: review?(standard8)
This fixes _A LOT_ of warnings inside tests.
Attachment #8627199 - Attachment is obsolete: true
Attachment #8627199 - Flags: review?(standard8)
Attachment #8627641 - Flags: review?(standard8)
(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.
Duplicate of this bug: 1178741
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).
Attachment #8627640 - Flags: review?(standard8) → review-
(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.
Attachment #8627197 - Flags: review?(standard8) → review+
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?
Attachment #8627641 - Flags: review?(standard8) → review+
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.
Attachment #8627640 - Attachment is obsolete: true
Attachment #8628184 - Flags: review?(standard8)
Carrying over r=Standard8
Attachment #8627197 - Attachment is obsolete: true
Attachment #8628185 - Flags: review+
Carrying over r=Standard8 (addressed review comments)
Attachment #8627641 - Attachment is obsolete: true
Attachment #8628186 - Flags: review+
Duplicate of this bug: 1172847
Iteration: 41.3 - Jun 29 → 42.1 - Jul 13
Sevaan, can you watch this video of about 3 minutes - https://vimeo.com/132418735 - and tell me what you think?
Flags: needinfo?(sfranks)
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.
Attachment #8628184 - Flags: review?(standard8) → review+
(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.
Flags: needinfo?(sfranks)
(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.
Attachment #8628184 - Attachment is obsolete: true
Attachment #8629285 - Flags: review?(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 ;-)
Attachment #8629285 - Flags: review?(standard8) → review+
Attached patch Patch 5: new tests (obsolete) — Splinter Review
Attachment #8629339 - Flags: review?(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"
Attachment #8629339 - Flags: review?(standard8)
I'm glad you mentioned that!
Attachment #8629339 - Attachment is obsolete: true
Attachment #8629352 - Flags: review?(standard8)
Attachment #8629352 - Flags: review?(standard8) → review+
QA Contact: bogdan.maris
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.
Attachment #8627195 - Flags: approval-mozilla-aurora?
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.
Attachment #8628185 - Flags: approval-mozilla-aurora?
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.
Attachment #8628186 - Flags: approval-mozilla-aurora?
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.
Attachment #8629285 - Flags: approval-mozilla-aurora?
Comment on attachment 8629352 [details] [diff] [review]
Patch 5.1: new tests

Please see comment 34 for the approval comment of this patch.
Attachment #8629352 - Flags: approval-mozilla-aurora?
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
Attachment #8627195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8628185 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8628186 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8629285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8629352 [details] [diff] [review]
Patch 5.1: new tests

Approving uplift to Aurora because verified in m-c, tests pass.
Attachment #8629352 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
Target Milestone: --- → mozilla42
Needs rebasing for Aurora uplift.
Flags: needinfo?(mdeboer)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47)
> Needs rebasing for Aurora uplift.

I was already working on it ;-) Thanks!
Flags: needinfo?(mdeboer)
Target milestone tracks trunk landing.
Target Milestone: mozilla41 → mozilla42
You need to log in before you can comment on or make changes to this bug.