Closed Bug 1215612 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(firefox45 unaffected, firefox46 verified, firefox47 verified)

VERIFIED FIXED
Iteration:
46.2 - Jan 11
Tracking Status
firefox45 --- unaffected
firefox46 --- verified
firefox47 --- verified

People

(Reporter: standard8, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance criteria:

- A change is defined as: a user navigates to a different domain, or a user changes to a tab which is a different domain.
- When in tab sharing, if a change occurs, then the new domain is inserted into the text chat area as a "context tile"
- The tile appears as the above screenshot.
- The acceptance criteria of bug 1215593 are not regressed (namely layout of initial tile and the context at room open doesn't appear on desktop).

Attachments

(6 files, 6 obsolete files)

Attached image Mock Up of layout (obsolete) —
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.
Rank: 19
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.
Flags: needinfo?(standard8)
Rank: 19 → 22
(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.
Flags: needinfo?(standard8)
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;
Attachment #8675041 - Attachment is obsolete: true
Assignee: nobody → b.mcb
Rank: 22 → 20
Status: NEW → ASSIGNED
Iteration: --- → 45.3 - Dec 14
Rank: 20 → 19
Priority: P2 → P1
WIP patch
Attachment #8694124 - Flags: feedback?(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.
Attachment #8694124 - Flags: feedback?(standard8) → feedback+
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!
Attachment #8697992 - Flags: review?(standard8)
Attachment #8694124 - Attachment is obsolete: true
Attached image Standalone version (obsolete) —
Attachment #8698392 - Flags: ui-review?(sfranks)
Attached image Link generator version (obsolete) —
Hey Sevaan can you check if the UI fits the designs?

Thanks!
Attachment #8698393 - Flags: ui-review?(sfranks)
UI updated
Attachment #8698403 - Flags: review?(standard8)
Attachment #8697992 - Attachment is obsolete: true
Attachment #8697992 - Flags: review?(standard8)
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?
Attachment #8698393 - Flags: ui-review?(sfranks) → ui-review-
Comment on attachment 8698392 [details]
Standalone version

The bubbles should have the same padding around it as the context tile above it.
Attachment #8698392 - Flags: ui-review?(sfranks) → ui-review-
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.
Attachment #8698403 - Flags: review?(standard8)
Attached file 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!
Attachment #8700491 - Flags: review?(standard8)
Attached image Link generator version
Attachment #8700492 - Flags: ui-review?(sfranks)
Attached image Standalone version
Attachment #8698392 - Attachment is obsolete: true
Attachment #8700493 - Flags: ui-review?(sfranks)
Attachment #8698393 - Attachment is obsolete: true
Attachment #8698403 - Attachment is obsolete: true
Blocks: 1232307
Comment on attachment 8700491 [details] [review]
PR

Please see comments on the PR.
Attachment #8700491 - Flags: review?(standard8)
Attachment #8700493 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8700492 - Flags: ui-review?(sfranks) → ui-review+
Rank: 19 → 11
Rank: 11 → 10
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.
Attachment #8703756 - Flags: review+
https://github.com/mozilla/loop/commit/538a2213db70a4f97ad68767b09611faa8a87ff5
Status: ASSIGNED → RESOLVED
Iteration: 45.3 - Dec 14 → 46.2 - Jan 11
Closed: 7 years ago
Resolution: --- → FIXED
Flags: qe-verify+
QA Contact: bogdan.maris
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.
Flags: needinfo?(b.mcb)
(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.
Flags: needinfo?(b.mcb)
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.