1174714 - Add the Loop room context information into the standalone text chat area

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

As part of the work on bug 1168829, I really need to be able to remove the old layouts on standalone for the context information.

I'm separating the work out, as its a good couple of separate patches, and we can use the ui-showcase to verify the layout.

The aim of this bug is:

- Display the Room Name and Context information in the standalone text area as per bug 1084991 attachment 8614721 [details]

I'm not covering changing of the existing standalone UI.

Comment 1

[Mark Banner (:standard8)]

Attachment: New layout after patches

I'm just about to attach the patches for this, but here's the current layout with the room name & context.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1. Drop obsolete box shadows in Loop's css.

This patch could probably go almost anywhere, but I found these that aren't being used any more and just wanted to get them out of the way first.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 2. Display the room name at the start of the text chat view in the Loop Standalone UI.

This patch adds the room name to the start of the text chat view for the standalone UI. It doesn't do it for desktop - we'll do that in bug 1171940.

The main idea I've gone for here is that the room name (and context) should be scrolling away when text chat starts to scroll. Hence, the idea to include them in the text chat entries as special entries. It'll also make sense for when we add more urls later which will also be displayed in a similar format to context.

This also adds a basic text chat view to the ui-showcase. It gets a little more work on it in the next patch. I'm aware you're doing work for text chat views as well, so I've just added something simple that's representative of the standalone UI for now.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 3. Display room context information in the Loop standalone UI text chat area.

This adds display of context just below the room name. The layout of elements is the same as those for the Loop panel in the new conversation area - hence I've moved the elements and styles into shared files and a new component. This should give us better re-use.

The text chat store is now extended for "Context" content types as well.

Note: to get the display to work in the ui-showcase, you need to remove the .chat-disabled style in webapp.css and change the "textChatEnabled: false" line to true in ui-showcase.js (both have bug 1168829 referenced next to them).

The reason for the manual extra work for testing, is that I'd done some implementation ready for display on the standalone UI (text chat will be displayed all the time by default), and I didn't really want to separate it out - but I wanted to have these patches in a landable state.

Obviously the text-chat element isn't laid out properly yet in the UI, the screenshot for Sevaan is with my incomplete patches from bug 1168829 applied.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8622478 [details] [diff] [review]
Part 1. Drop obsolete box shadows in Loop's css.

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

r=dmose

Comment 6

[Sevaan Franks [:sevaan]]

Comment on attachment 8622473 [details]
New layout after patches

+r, but is it possible to move the favicon up 2px to better align with the first line of text (e.g. http://i.sevaan.com/image/392G2A2h1Z0d)?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8622483 [details] [diff] [review]
Part 2. Display the room name at the start of the text chat view in the Loop Standalone UI.

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

Feel free to convince me that this is the right strategy rather than necessarily changing the hiding vs. rendering stuff.  r- for now, but I'm open to changing that if appropriate...

::: browser/components/loop/content/js/roomViews.jsx
@@ +762,5 @@
>                  roomData={roomData}
>                  show={!shouldRenderInvitationOverlay && shouldRenderContextView} />
> +              <sharedViews.TextChatView
> +                dispatcher={this.props.dispatcher}
> +                standalone={false} />

I'm not a huge fan of defining this in terms of the embedding context rather than the features enabled.  How about doing that instead?

::: browser/components/loop/content/shared/js/textChatStore.js
@@ +91,5 @@
>        var newList = this._storeState.messageList.concat(message);
>        this.setStoreState({ messageList: newList });
>  
> +      if (type != CHAT_MESSAGE_TYPES.SPECIAL) {
> +        window.dispatchEvent(new CustomEvent("LoopChatMessageAppended"));

In some ideal world, it seems like MozLoopService shouldn't need to manually be told about contain changes in order for it to size the window correctly, but I'll assume that it is (though I'd love to know way).

In any case, a comment here explaining why this event is being dispatched would be great, particularly since it's easy to mistake for an action on a fast skim.

@@ +130,5 @@
> +    updateRoomInfo: function(actionData) {
> +      this._appendTextChatMessage(CHAT_MESSAGE_TYPES.SPECIAL, {
> +        contentType: CHAT_CONTENT_TYPES.ROOM_NAME,
> +        message: mozL10n.get("rooms_welcome_title", {conversationName: actionData.roomName})
> +      });

What happens if the room is renamed after some chat has already happened?  My gut is that this would just get appended and look a little strange, but maybe this can't happen in practice or something.  If it can't, some sort of assertion to the console would be nice.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +71,5 @@
>  
>      render: function() {
> +      var chatListClasses = React.addons.classSet({
> +        "text-chat-entries": true,
> +        "no-entries": !this.props.messageList.length

This no-entries thing seems a bit add.  Presumably, it's necessary so that one doesn't count the "special" piece as an entry?  Why not count it as an entry, given that it scrolls away like one?

Part my reaction, I guess, is that with React, it seems a bit strange to render something and then hide it, rather than simply avoiding rendering it, like the code was doing before...

@@ +161,5 @@
> +      var chatViewClasses = React.addons.classSet({
> +        "text-chat-view": true,
> +        "chat-disabled": !this.state.textChatEnabled,
> +        "no-entries": !messageList.length
> +      });

Similar question about rendering and hiding to the one above.

::: browser/components/loop/standalone/content/css/webapp.css
@@ -370,5 @@
>  
>  .text-chat-entries {
>    /* XXX Should use flex, this is just for the initial implementation. */
>    height: calc(100% - 2em);
> -  width: 30%;

Why this deletion?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8622486 [details] [diff] [review]
Part 3. Display room context information in the Loop standalone UI text chat area.

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

Looks good, r=dmose with appropriate comments addressed.

::: browser/components/loop/content/js/panel.jsx
@@ +772,5 @@
>              <Checkbox label={mozL10n.get("context_inroom_label")}
>                        onChange={this.onCheckboxChange} />
> +            <sharedViews.ContextUrlView
> +              description={this.state.description}
> +              isDesktop={true}

Ditching the isDesktop in favor of something semantic about what behavior is effected would again be nice, if practical.

::: browser/components/loop/content/shared/css/common.css
@@ +502,5 @@
>    background-image: url("../img/check.svg#check-disabled");
>  }
> +
> +/* ContextUrlView classes */
> +

This section feels like it needs a few more comments.  Particularly the why the flex values are what they are, as well as where the 16px stuff comes from.  Though it looks like this was just moved from elsewhere, in which case I suppose we can grandfather it in.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1276,5 @@
>    border-color: #0095dd;
>    border-radius: 10000px;
>    padding: .5em 1em;
> +  margin: 0;
> +  display: inline-block;

I'm surprised this needs to be inline block (rather than just block), since I'm not imagining we'd ever have more than one of these on a line at once.  Am I missing something?  If so, a comment explaining would be nice.

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +195,4 @@
>            <div className={textChatBoxClasses}>
>              <form onSubmit={this.handleFormSubmit}>
>                <input type="text"
> +                     placeholder={messageList.length ? "" : mozL10n.get("chat_textbox_placeholder")}

Good catch.  Did this just never work before?

::: browser/components/loop/content/shared/js/views.jsx
@@ +694,5 @@
> +   */
> +  var ContextUrlView = React.createClass({
> +    mixins: [React.addons.PureRenderMixin],
> +
> +    PropTypes: {

Each of these properties wants a documentation comment, I think.  Could all happen in a single jsdoc block above PropTypes or even the class (I think usejsdoc.org shows syntax for that.

@@ +697,5 @@
> +
> +    PropTypes: {
> +      allowClick: React.PropTypes.bool,
> +      description: React.PropTypes.string,
> +      dispatcher: React.PropTypes.instanceOf(loop.Dispatcher),

This (and maybe some others?) wants isRequired, I bet.

@@ +741,5 @@
> +
> +      var thumbnail = this.props.thumbnail;
> +
> +      if (!thumbnail) {
> +        thumbnail = this.props.isDesktop ?

Maybe just pass in the thumbnail SVG path as a prop instead of the isDesktop thing?

::: browser/components/loop/test/shared/textChatStore_test.js
@@ +162,5 @@
> +          thumbnail: "fake"
> +        }]
> +      }));
> +
> +      expect(store.getStoreState("messageList")).eql([{

This would be marginally easier to read if the [{ started a line by itself.

Comment 9

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #7) 
> @@ +130,5 @@
> > +    updateRoomInfo: function(actionData) {
> > +      this._appendTextChatMessage(CHAT_MESSAGE_TYPES.SPECIAL, {
> > +        contentType: CHAT_CONTENT_TYPES.ROOM_NAME,
> > +        message: mozL10n.get("rooms_welcome_title", {conversationName: actionData.roomName})
> > +      });
> 
> What happens if the room is renamed after some chat has already happened? 

For standalone: nothing, as it doesn't receive notifications of updates.

For desktop this would show it multiple times, however it currently doesn't get displayed so to save on the amount of work for these patches, I'm punting fixing it to bug 1171940.

> ::: browser/components/loop/content/shared/js/textChatView.jsx
> >      render: function() {
> > +      var chatListClasses = React.addons.classSet({
> > +        "text-chat-entries": true,
> > +        "no-entries": !this.props.messageList.length
> 
> This no-entries thing seems a bit add.  Presumably, it's necessary so that
> one doesn't count the "special" piece as an entry?  Why not count it as an
> entry, given that it scrolls away like one?

It was mainly because of specialisms in display between standalone & desktop, which seemed at the time slightly easier managing in css than doing it in code. However, its a little bit of a toss up as to which is better, so I've changed it to the code form.

> ::: browser/components/loop/standalone/content/css/webapp.css
> @@ -370,5 @@
> >  
> >  .text-chat-entries {
> >    /* XXX Should use flex, this is just for the initial implementation. */
> >    height: calc(100% - 2em);
> > -  width: 30%;
> 
> Why this deletion?

That was a temporary thing for the standalone view. It was getting in the way of testing the standalone views and in the way of the ui-showcase, so it seemed best to just drop it.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 2. Display the room name at the start of the text chat view in the Loop Standalone UI.

Updated part 2 for review comments.

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 3. Display room context information in the Loop standalone UI text chat area.

Updated part 3 patch for reference.

Comment 12

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #8)
> ::: browser/components/loop/content/shared/css/conversation.css
> @@ +1276,5 @@
> >    border-color: #0095dd;
> >    border-radius: 10000px;
> >    padding: .5em 1em;
> > +  margin: 0;
> > +  display: inline-block;
> 
> I'm surprised this needs to be inline block (rather than just block), since
> I'm not imagining we'd ever have more than one of these on a line at once. 
> Am I missing something?  If so, a comment explaining would be nice.

inline-block stops the elements taking up 100% width of the view. The change from span to p caused this, but we needed to do that for the multiple line text to work correctly.

> ::: browser/components/loop/content/shared/js/textChatView.jsx
> > +                     placeholder={messageList.length ? "" : mozL10n.get("chat_textbox_placeholder")}
> 
> Good catch.  Did this just never work before?

I changed the variable name in the file from "mozl10n" to "mozL10n" to match what we do elsewhere. It is currently only used in this place.

> @@ +697,5 @@
> > +
> > +    PropTypes: {
> > +      allowClick: React.PropTypes.bool,
> > +      description: React.PropTypes.string,
> > +      dispatcher: React.PropTypes.instanceOf(loop.Dispatcher),
> 
> This (and maybe some others?) wants isRequired, I bet.

Its only required if allowClick is specified. So I didn't want to enforce lots of things just so we have to list lots of unnecessary things for each time its used. I have made a few of the options to be required though.

> @@ +741,5 @@
> > +      if (!thumbnail) {
> > +        thumbnail = this.props.isDesktop ?
> 
> Maybe just pass in the thumbnail SVG path as a prop instead of the isDesktop
> thing?

I went with useDesktopPaths for now. Splitting the location of the fallback thumbnail seems wrong. Whilst we could pass in a path, I don't think it'd fix it all nicely.

Comment 13

[Mark Banner (:standard8)]

Attachment: Part 3. Display room context information in the Loop standalone UI text chat area.

Corrected part 3 (I'd forgotten to re-disable the text chat views for the standalone for being able to land this bug).

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8623007 [details] [diff] [review]
Part 2. Display the room name at the start of the text chat view in the Loop Standalone UI.

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

Looks good, I've suggested a couple of tweaks.  Also note that with all three patches applied, the globe icon is broken in the ui-showcase now, though if you want to fix that later, that's fine.

r=dmose

::: browser/components/loop/content/shared/js/textChatView.jsx
@@ +103,2 @@
>     */
> +  var TextChatBoxView = React.createClass({

How about naming this TextChatInputView for more clarity?

@@ +219,5 @@
>            <TextChatEntriesView messageList={messageList} />
> +          <TextChatBoxView
> +            dispatcher={this.props.dispatcher}
> +            showPlaceholder={!hasNonSpecialMessages}
> +            textChatEnabled={this.state.textChatEnabled} />

I like that this has been hoisted into it's own component; it improves readability nicely.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +1090,5 @@
>        activeRoomStore: activeRoomStore
>      });
>      var textChatStore = new loop.store.TextChatStore(dispatcher, {
> +      sdkDriver: sdkDriver,
> +      standalone: true

What's the "standalone" for?  From what I can see, the store itself doesn't do anything with that.

Comment 15

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #14)
> Looks good, I've suggested a couple of tweaks.  Also note that with all
> three patches applied, the globe icon is broken in the ui-showcase now,
> though if you want to fix that later, that's fine.

Yeah, globe icon is broken, we're going to have to fix it later. I have potential ways, but want to investigate a bit.

> ::: browser/components/loop/standalone/content/js/webapp.jsx
> @@ +1090,5 @@
> >        activeRoomStore: activeRoomStore
> >      });
> >      var textChatStore = new loop.store.TextChatStore(dispatcher, {
> > +      sdkDriver: sdkDriver,
> > +      standalone: true
> 
> What's the "standalone" for?  From what I can see, the store itself doesn't
> do anything with that.

Oops, that was something old. Now removed.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3ce5e13f9693
https://hg.mozilla.org/integration/fx-team/rev/5e6221183248
https://hg.mozilla.org/integration/fx-team/rev/c171d6b6b7a6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ce5e13f9693
https://hg.mozilla.org/mozilla-central/rev/5e6221183248
https://hg.mozilla.org/mozilla-central/rev/c171d6b6b7a6