1142587 - Implement indicators for context in conversations in the panel alongside room names

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Implementation bug for implementing the indicators for context for conversations

Comment 1

[Mark Banner (:standard8)]

I have a patch almost ready for this.

Sevaan: one question, should we open the url in a tab when you click on the context indicator?

Comment 2

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #1)
> Sevaan: one question, should we open the url in a tab when you click on the
> context indicator?

Comment 3

[Mark Banner (:standard8)]

Attachment: Implement indicators for context in conversations in the panel alongside room names.

Basically complete, depends on patch in bug 1154708. Needs answer to UX question.

Comment 4

[Sevaan Franks [:sevaan]]

(In reply to Mark Banner (:standard8) from comment #2)
> (In reply to Mark Banner (:standard8) from comment #1)
> > Sevaan: one question, should we open the url in a tab when you click on the
> > context indicator?

Yes please. A new tab, actually.

Comment 5

[Mark Banner (:standard8)]

Attachment: Implement indicators for context in conversations in the panel alongside room names.

This finishes the new indicator and makes it clickable for opening the link.

I've assumed 16x16 for the icon as we seem to be heading towards favicons.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8595336 [details] [diff] [review]
Implement indicators for context in conversations in the panel alongside room names.

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

::: browser/components/loop/content/css/panel.css
@@ +365,5 @@
> +}
> +
> +.room-entry-context-item > a > img {
> +  height: 16px;
> +  width: 16px;

I *think* that if you specify only the width here, it'll resize the source img maintaining aspect ratio.

::: browser/components/loop/content/js/panel.jsx
@@ +443,5 @@
> +      this.props.mozLoop.openURL(event.currentTarget.href);
> +    },
> +
> +    render: function() {
> +      if (!this.props.roomUrls || !this.props.roomUrls.length ||

I vote for declaring a shorthand:

```js
var roomUrl = this.props.roomUrls && this.props.roomUrls[0];
if (!roomUrl) {
  return null;
}
...
```

@@ +444,5 @@
> +    },
> +
> +    render: function() {
> +      if (!this.props.roomUrls || !this.props.roomUrls.length ||
> +          !this.props.roomUrls[0].location) {

How can this last one ever be the case?

Comment 7

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> > +.room-entry-context-item > a > img {
> > +  height: 16px;
> > +  width: 16px;
> 
> I *think* that if you specify only the width here, it'll resize the source
> img maintaining aspect ratio.

Right, but I don't want to do that. Given the expected switch to favicons, I think keeping it square is fine and I'd rather be explicit here. If we change, then we can come back and revisit.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7be3f9bb88bf

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7be3f9bb88bf

Comment 10

[Bogdan Maris, Desktop Test Engineering]

Verified that the indicators for context are properly displayed in panel alongside room names, user story checks with the implementation. Tested using latest Aurora on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.