Rework link-generator display for text chat

VERIFIED FIXED in Firefox 41

Status

defect
P1
normal
Rank:
13
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: mikedeboer)

Tracking

unspecified
mozilla41
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [chat])

User Story

Approximate UX layout: http://i.sevaan.com/image/0I0Z0o1a0346

Rework the existing display such that:

- If chat is enabled, but no text has been entered, then the text entry box is shown at the bottom of the window
-- The height of the window is increased for the text entry box
- If chat is enabled, and text has been entered, then the text display and entry boxes are shown at the bottom of the window
-- The height of the window is increased again for both boxes.

If easily possible in this bug, an animation effect should be used for increasing the window height.

Not included:

- Layout work within the text entry/text boxes
- Changing the layout of anything else within the existing conversation window

Attachments

(1 attachment, 5 obsolete attachments)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Points: --- → 8
Rank: 20
Whiteboard: [chat]
Rank: 20 → 21
Assignee: nobody → mdeboer
Iteration: --- → 41.2 - Jun 8
Video that show the above patch in action: https://vimeo.com/129664555
Status: NEW → ASSIGNED
Blocks: 1168841
Attachment #8614664 - Attachment is obsolete: true
Attachment #8615237 - Flags: review?(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.
Attachment #8615237 - Flags: review?(standard8) → review-
Blocks: 1171940
Blocks: 1171942
(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.
I *think* this patch addresses all your concerns listed above... would you like to check it out?
Attachment #8615237 - Attachment is obsolete: true
Attachment #8616044 - Flags: feedback?(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.
Attachment #8616044 - Flags: feedback?(standard8) → feedback+
Rank: 21 → 13
Priority: P2 → P1
Attachment #8616044 - Attachment is obsolete: true
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
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?
Attachment #8617287 - Flags: review?(standard8) → review-
Attachment #8617287 - Flags: feedback?(mixedpuppy)
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! ;-)
Review comments addressed.
Attachment #8617287 - Attachment is obsolete: true
Attachment #8617287 - Flags: feedback?(mixedpuppy)
Attachment #8620958 - Flags: review?(standard8)
Now with test fixes!
Attachment #8620958 - Attachment is obsolete: true
Attachment #8620958 - Flags: review?(standard8)
Attachment #8621050 - Flags: review?(standard8)
Comment on attachment 8621050 [details] [diff] [review]
Patch v5: Revamp base text-chat styles in conversation window

Looks good, ship it!
Attachment #8621050 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/56378c101d30
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.