1168833 - Rework link-generator display for text chat

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

For text chat, we need to rework the desktop layout as required by the latest mock-ups (to be attached).

See user story for what needs doing.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: WIP: text chat in conversation window feature complete

Comment 2

[Mike de Boer [:mikedeboer]]

Video that show the above patch in action: https://vimeo.com/129664555

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: Revamp base text-chat styles in conversation window

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8615237 [details] [diff] [review]
Patch v1: Revamp base text-chat styles in conversation window

Review of attachment 8615237 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good start. I noticed a few issues though.

- I think we should get a build to Sevaan and get his ui-review on it. Particular concerns:
-- When we first open the window, its now expanding in both directions. Feels a bit strange.
--- This also changes the default height of the window, before the text box is displayed (I think the original intent was that it'd be increased only when the text box was displayed).
-- If you pop-out the window and then back-in again, it goes to a small size and transitions back out. This feels strange.
- Other issues when I pop-out the window:
-- having the window width greater than the height, then the local video pops down over the text chat.
-- in this state, receiving a message causes the text box to be hidden under the bottom of the window. You can get it back again by resizing, but then entering more items causes it to disappear again.
-- Its also possible to hide the text chat sections completely by making the window too short.
-- I also got it in the state once, where putting the window back-in to the main window, caused the text chat to remain hidden. I can't reproduce that at the moment.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1314,4 @@
>  }
>  
>  .text-chat-entry {
> +  text-align: right;

I think we need to do rtl version of this.

@@ +1330,1 @@
>    text-align: left;

Probably rtl here as well.

::: browser/components/loop/content/shared/js/textChatStore.js
@@ +70,5 @@
>        this.setStoreState({ textChatEnabled: true });
>      },
>  
>      /**
> +     * Appends a message to the store, 

eslint says no trailing spaces - although this looks like an incomplete jsdoc.

@@ +86,5 @@
> +      };
> +      var newList = this._storeState.messageList.concat(message);
> +      this.setStoreState({ messageList: newList });
> +
> +      window.dispatchEvent(new CustomEvent("LoopChatMessageAppended"));

Should we limit this to only happen for desktop and not the standalone client?

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +74,3 @@
>        return (
>          <div className="text-chat-entries">
> +          <div className="text-chat-scroller">

What's the reason for this additional div? Maybe add a comment so that we know if it is needed.

::: browser/components/loop/test/shared/textChatStore_test.js
@@ +108,5 @@
>          message: messageData.message
>        }]);
>      });
> +
> +    it("should dipatch a LoopChatMessageAppended event", function() {

nit: dispatch

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +356,5 @@
>  context_learn_more_link_label=Learn more.
> +
> +# Text chat strings
> +
> +chat_textbox_placeholder=Type here…

Please add this to the standalone loop.properties as well.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #4)
> Should we limit this to only happen for desktop and not the standalone
> client?

Not sure, I don't think the standalone client will choke on it, so doesn't really hurt...

> What's the reason for this additional div? Maybe add a comment so that we
> know if it is needed.

Well, this is really a (premature) optimization to let a single element grow inside the overflow parent. This should/ could reduce paint times. However, I'm happy to take it out and see about it later when we hit a problem like this.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: Revamp base text-chat styles in conversation window

I *think* this patch addresses all your concerns listed above... would you like to check it out?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8616044 [details] [diff] [review]
Patch v2: Revamp base text-chat styles in conversation window

Review of attachment 8616044 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is definitely an improvement, there's still a couple of issues, that I don't think have been addressed from what I can tell:

- When you pop-out the window, having the window width greater than the height, then the local video pops down over the text chat.
-- in this state, receiving a message causes the text box to be hidden under the bottom of the window. You can get it back again by resizing, but then entering more items causes it to disappear again.
- If you pop-out the window and then back-in again, it goes to a small size and transitions back out. This feels strange.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: Revamp base text-chat styles in conversation window

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8617287 [details] [diff] [review]
Patch v3: Revamp base text-chat styles in conversation window

Review of attachment 8617287 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely a lot better on layout. A few comments on the css to be worked through though.

::: browser/base/content/browser.css
@@ +906,5 @@
>    height: 285px;
>    width: 260px; /* CHAT_WIDTH_OPEN in socialchat.xml */
>  }
>  
> +chatbar[sizemode="1"] > .chatbar-innerbox {

I think we should change these size mode to be text names if possible. The numbers are not really self-documenting, and its hard to follow what is what - without referring back & forth to MozLoopService.

Might also be worth a general note that the *sizemode attributes are used by Loop.

@@ +907,5 @@
>    width: 260px; /* CHAT_WIDTH_OPEN in socialchat.xml */
>  }
>  
> +chatbar[sizemode="1"] > .chatbar-innerbox {
> +  margin-top: -425px;

This needs an explanation. I can't see margin-top for any other mode nor in any of the existing styles,

@@ +914,5 @@
> +chatbox[sizemode] {
> +  width: 300px; /* CHAT_WIDTH_OPEN_ALT in socialchat.xml */
> +}
> +
> +#chat-window[chatSizemode] {

Its strange using the sizemode vs chatSizemode here, especially when they are representing the same thing. Maybe use sizemode for both? If not, maybe windowSizemode would be better to indicate that its to do with the popped out window.

@@ +919,5 @@
> +  min-width: 300px;
> +}
> +
> +chatbox[sizemode="2"] {
> +  height: 325px;

Is this value just from UX, or does it represent a calculation based on the Loop styles? Probably worth documenting either way.

@@ +924,5 @@
> +}
> +
> +#chat-window[chatSizemode="2"] {
> +  /* 325px + 30px top bar height. */
> +  min-height: 355px;

I'd be tempted to use calc here, so that its easier to pick up visually if we changed the basic height of sizemode 2 for instance, and it means the human factor of miss-calculation on adjustment is less likely.

@@ +928,5 @@
> +  min-height: 355px;
> +}
> +
> +chatbox[sizemode="3"] {
> +  height: 445px;

ditto comment re where this comes from.

@@ +933,5 @@
> +}
> +
> +#chat-window[chatSizemode="3"] {
> +  /* 445px + 30px top bar height. */
> +  min-height: 475px;

ditto re calc comment.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1227,5 @@
>  }
>  
> +/* Text chat in rooms styles */
> +
> +.fx-embedded .room-conversation-wrapper {

I was thinking of suggesting we make these styles more generic between the two, but now I've looked at it, I don't think it makes sense to do that just yet.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +28,5 @@
>        });
>  
>        return (
>          <div className={classes}>
> +          <span>{this.props.message}</span>

This file is missing the .js diff...

@@ +77,1 @@
>            {

nit: would prefer the sub-section indented by 2 more spaces.

::: browser/components/loop/modules/MozLoopService.jsm
@@ +893,5 @@
> +            chatbox.setAttribute("sizemode", sizemode);
> +            // Propagate the sizemode attribute on the window too when the chat
> +            // was detached earlier.
> +            if (chatbox.parentNode.hasAttribute("chatSizemode")) {
> +              chatbox.parentNode.setAttribute("chatSizemode", sizemode);

This really does feel like a bit of duplication, especially if I understand the socialchat.xml changes right - as isn't that copying the "sizemode" attribute across when popping out the window?

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8617287 [details] [diff] [review]
Patch v3: Revamp base text-chat styles in conversation window

Shane, can you take a look at the socialchat.xml, MozLoopService.jsm and browser.css changes? Perhaps you know of a better way to implement variadic sizes of the chat window and/ or know better names/ mechanisms that I've invented here?

Any and all ideas welcome! ;-)

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: Revamp base text-chat styles in conversation window

Review comments addressed.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch v5: Revamp base text-chat styles in conversation window

Now with test fixes!

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8621050 [details] [diff] [review]
Patch v5: Revamp base text-chat styles in conversation window

Looks good, ship it!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/56378c101d30

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56378c101d30

Comment 16

[Bogdan Maris, Desktop Test Engineering]

I did some exploratory testing around this fix using Firefox Developer Edition 41.0a2 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and I can confirm the layout implementation is according to user story, found no new issue so I'll mark this as verified fixed.