1215612 - Add a context tile to the text chat every time context changes

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Mock Up of layout

As part of the user journey rework, we need to move and adjust the availability option. This depends on bug 1215593 for the new context tile layout, and bug 1215570 for the initial hooking up of change of context on domain/tab change.

See the user story for more detail.

Comment 1

[Romain Testard [:RT]]

We had "Conversation window: Change room name and favicon in conversation window title bar as I switch tabs" as P3 on bug 1209713.
Perhaps it makes sense to implement this P3 together with this bug since both require the same strings/Favicon? If it does not make it simpler to address both at once let's forget about it since it was fairly low priority and we can address this at a later stage.

Comment 2

[Mark Banner (:standard8)]

(In reply to Romain Testard [:RT] from comment #1)
> Perhaps it makes sense to implement this P3 together with this bug since
> both require the same strings/Favicon? If it does not make it simpler to
> address both at once let's forget about it since it was fairly low priority
> and we can address this at a later stage.

Whilst they're similar they are slightly different code paths, and this one needs more css. I've cc'ed Ed for awareness as he's working on bug 1215570 which I think is the related one you're talking about.

Comment 3

[Pau Masiá [:Pau]]

Attachment: Updated Context Tile Mockup

The Mock you uploaded is a bit outdated, Mark. Here's the good one. I think there's no need for devs to work twice styling the same element because they were given the wrong mocks/specs.

Since the design (specially visual one) is changing and Sevaan and I are the ones who know the status of it, please could you ask us before uploading something to be implemented? 

- Mock specs:

/* Rectangle: */
width: 254px;
height: 50px;
background: #FFFFFF;
border: 1px solid rgba(0,0,0,0.10);
border-radius: 4px;
padding: 10px;

/* Home of the Mozilla : */
font-family: Lucida grande;
font-size: 12px;
color: #000000;
line-height: 16px;

Comment 5

[Manuel Casas Barrado [:mancas]]

Attachment: Add a context tile to the text chat every time context changes.

WIP patch

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8694124 [details] [diff] [review]
Add a context tile to the text chat every time context changes.

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

Looks reasonable so far, a few comments below.

::: browser/extensions/loop/content/shared/js/textChatStore.js
@@ +16,5 @@
>      SPECIAL: "special"
>    };
>  
>    var CHAT_CONTENT_TYPES = loop.shared.utils.CHAT_CONTENT_TYPES;
> +  var DOMAIN_REG_EXP = "/^https?\:\/\/([^\/?#]+)(?:[\/?#]|$)/i/";

This will need breaking down and documenting what all the parts are, a bit like what's been done in urlRegExps.js

@@ +199,5 @@
> +     * @param  {sharedActions.updateRoomContext} actionData
> +     */
> +    updateRoomContext: function(actionData) {
> +console.info("MANU", actionData);
> +      // Firstly, check if there is a previous context tile, if not, create it

I don't understand why we'd need to do this. It doesn't matter if there's a context tile already existing or not, we just create a new one with whatever we have available.

@@ +224,5 @@
> +
> +      this._appendContextTileMessage(actionData);
> +     },
> +
> +     _appendContextTileMessage: function(data) {

we need to find a way of refactoring this function and the existing _appendTextChatMessage one - when adding a context tile, we'll need to send the LoopChatMessageAppended/LoopChatDisabledMessageAppended in case we're in the standalone side of things.

Comment 7

[Manuel Casas Barrado [:mancas]]

Attachment: Add a context tile to the text chat every time context changes.

Hey Mark, I've updated the patch according to our last talk in Orlando. Visuals need to be reviewed with Pau but in the meantime you can start the review.

Thanks!

Comment 8

[Manuel Casas Barrado [:mancas]]

Attachment: Standalone version

Comment 9

[Manuel Casas Barrado [:mancas]]

Attachment: Link generator version

Hey Sevaan can you check if the UI fits the designs?

Thanks!

Comment 10

[Manuel Casas Barrado [:mancas]]

Attachment: Add a context tile to the text chat every time context changes.

UI updated

Comment 11

[Sevaan Franks [:sevaan]]

Comment on attachment 8698393 [details]
Link generator version

That booking feels like it has a lot of room on the left side to expand more, reducing the text wrapping. Is this possible?

Comment 12

[Sevaan Franks [:sevaan]]

Comment on attachment 8698392 [details]
Standalone version

The bubbles should have the same padding around it as the context tile above it.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8698403 [details] [diff] [review]
Add a context tile to the text chat every time context changes.

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

This is looking good so far. I've not looked at the css yet, until you get ui-r+ for that (or at least another update round).

Also, you need to add tests for the changes in textChatStore and textChatView please. Covering all the scenarios of updateRoomContext would be especially useful.

::: browser/extensions/loop/content/shared/js/activeRoomStore.js
@@ +926,5 @@
>  
>        // The browser being shared changed, so update to the new context
>        loop.request("GetSelectedTabMetadata").then(function(meta) {
> +        // Avoid sending the event if there is no data nor participants nor url
> +        if (!meta || !meta.url || !this._hasPaticipants()) {

Do we really want the hasParticipants check here? This would prevent the store room data being updated as well, wouldn't it? (which I assume we do want even if no-one is in the room, but we might need to check with Sevaan).

@@ +931,1 @@
>            return;

Please add a couple of tests for the !url and !_hasParticipants cases so that we can ensure we don't regress them in future.

@@ +940,5 @@
>              newRoomDescription: meta.title || meta.description || meta.url,
>              newRoomThumbnail: meta.favicon,
>              newRoomURL: meta.url,
> +            roomToken: this.getStoreState().roomToken,
> +            sentTimestamp: (new Date()).toISOString()

Seeing this, I think it might be better if we made textChatStore add the timestamp. It feels a little strange to set it outside.

What do you think?

@@ +1234,5 @@
>      sendTextChatMessage: function(actionData) {
>        this._handleTextChatMessage(actionData);
> +    },
> +
> +    _hasPaticipants: function() {

Please add jsdoc.

Also there's a typo here.

Comment 14

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1215612 > mozilla:master

Comment 15

[Manuel Casas Barrado [:mancas]]

Attachment: PR

Comments addressed. In a minute I'll upload new screenshots of the css changes so feel free to take a look at the patch or wait until Sevaan gives the ui-r+

Thanks!

Comment 16

[Manuel Casas Barrado [:mancas]]

Attachment: Link generator version

Comment 17

[Manuel Casas Barrado [:mancas]]

Attachment: Standalone version

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8700491 [details] [review]
PR

Please see comments on the PR.

Comment 19

[GitHub Autolander Bot]

Attachment: [loop] Standard8:bug1215612 > mozilla:master

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8703756 [details] [review]
[loop] Standard8:bug1215612 > mozilla:master

Manu: As you're out at the moment, I addressed the review comments and switched to a new PR so that we could land this.

Comment 21

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/538a2213db70a4f97ad68767b09611faa8a87ff5

Comment 22

[Bogdan Maris, Desktop QA]

Is there a reason why the tiles don't appear in StandaloneUI? I used both default server and stage. Could be that the Hello update was not pushed to Nightly yet? I know that Mark (or some other guys) made a push with the up to date (at that time) Hello in Nightly from 2016-01-15.

Comment 23

[Mark Banner (:standard8)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #22)
> Is there a reason why the tiles don't appear in StandaloneUI? I used both
> default server and stage. Could be that the Hello update was not pushed to
> Nightly yet? I know that Mark (or some other guys) made a push with the up
> to date (at that time) Hello in Nightly from 2016-01-15.

We haven't yet updated any of our standalone UIs (development or production) since we moved to github. We'll hopefully get the standalone UI updated this week or maybe next.

Comment 24

[Bogdan Maris, Desktop QA]

Verified that a context tile is properly displayed in both conversation window and standalone chat and changes each time a user switches the shared tab.
We used latest Nightly 47.0a1 build and latest Developer Edition 46.0a1 build with loop.server set to 'https://loop-dev.stage.mozaws.net/v0', across platforms (Windows 10 64-bit, Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit).