Closed Bug 1142514 Opened 5 years ago Closed 5 years ago

Add ability to view conversation context information from the conversation window

Categories

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

defect
Points:
5

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: standard8, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [context])

User Story

UX: https://bugzilla.mozilla.org/attachment.cgi?id=8563677
Updated UX: http://cl.ly/image/10431Y0u3V3r

* When in a conversation (alone or not), the following details about the attached URL get displayed to the desktop client user:
- Thumbnail and Favicon of the webpage the URL points to
- Content of the <title> tag of the webpage the URL points to
- URL
* The contextual information in the conversation panel can be closed through the close button (next time the conversation is opened these details will re-appear)
* Clicking the URL opens the URL in a new tab

Attachments

(4 files, 5 obsolete files)

Part of the work for bug 1115342 - display the received conversation data in the conversation view.
User Story: (updated)
Blocks: 1142515
Depends on: 1142522
No longer depends on: 1141133
Rank: 6
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [context]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
Points: --- → 5
Attachment #8584495 - Attachment description: Patch 1: Part 1 - update Loop localization notes to not contain URLs to the obsolete UX specification anymore → Patch 1: update Loop localization notes to not contain URLs to the obsolete UX specification anymore
Attachment #8584497 - Flags: review?(gavin.sharp)
Attachment #8584495 - Flags: review?(gavin.sharp) → review+
Comment on attachment 8584497 [details] [diff] [review]
Patch 2: add label caption for context information

This seems like a strange string out of context. Is there supposed to be a colon at the end? Don't we want something more like:

context_inroom_label=Let's talk about: {{url}}

or somesuch?
Well, that doesn't really work with the design as proposed at https://bug1115342.bugzilla.mozilla.org/attachment.cgi?id=8563677 - it really stands out as a separate string as the title & URL are positioned quite a bit below it.
Comment on attachment 8584497 [details] [diff] [review]
Patch 2: add label caption for context information

Well, that mockup does have a colon in it. So at the very least you need to add a colon here, and an l10n note explaining the context (e.g. "this will be placed before the title of the website you are having a conversation about").

Francesco might have an opinion about whether some locales might have a problem with that layout restriction.
Attachment #8584497 - Flags: review?(gavin.sharp)
Attachment #8584497 - Flags: review-
Attachment #8584497 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8584497 [details] [diff] [review]
Patch 2: add label caption for context information

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

I think locales should be able to work around this, but need a very detailed localization comment.

Example.

# LOCALIZATION NOTE (context_inroom_label): this string is followed by the
# title/URL of the website you are having a conversation about, displayed on a
# separate line. If this structure doesn't work for your locale, you might want
# to consider this as a stand-alone title. See example screenshot:
# https://bug1115342.bugzilla.mozilla.org/attachment.cgi?id=8563677
Attachment #8584497 - Flags: feedback?(francesco.lodolo) → feedback-
(In reply to Francesco Lodolo [:flod] (UTC+1) from comment #6)
> Example.
> 
> # LOCALIZATION NOTE (context_inroom_label): this string is followed by the
> # title/URL of the website you are having a conversation about, displayed on
> a
> # separate line. If this structure doesn't work for your locale, you might
> want
> # to consider this as a stand-alone title. See example screenshot:
> # https://bug1115342.bugzilla.mozilla.org/attachment.cgi?id=8563677

I like this example. Does this mean that if I'd take it this way, you'd turn f- into f+? Otherwise I don't quite understand the meaning of your f-...

I'd be grateful for the clarification!
Flags: needinfo?(francesco.lodolo)
The best solution would be the one proposed by Gavin, and that's the one I'd ask for in a normal context.

> context_inroom_label=Let's talk about: {{url}}

With this approach localizers can move around the placeholder as they see fit.

But we can't have that with the proposed layout, so I'm fine with the current string as long as there's a comment explaining how it's used and how to eventually cope with it (i.e. it's an f+ with the comment).
Flags: needinfo?(francesco.lodolo)
Carrying over f=flod, as per comment 8.
Attachment #8584497 - Attachment is obsolete: true
Attachment #8585160 - Flags: review?(gavin.sharp)
Attachment #8585160 - Flags: feedback+
Comment on attachment 8585160 [details] [diff] [review]
Patch 2.1: add label caption for context information

Punting to Mark, as we'd like to land this now.
Attachment #8585160 - Flags: review?(gavin.sharp) → review?(standard8)
Attachment #8585160 - Flags: review?(standard8) → review+
Keywords: leave-open
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
User Story: (updated)
Rank: 6 → 5
Attachment #8592196 - Flags: feedback?(standard8)
Comment on attachment 8592196 [details] [diff] [review]
Patch 3: show context information in the conversation window

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

I've only taken a brief look through at this stage and just put down some highlights. I think we need to agree on what we want to do in activeRoomStore.js/action.js and then it'll get a bit easier.

::: browser/components/loop/content/js/conversation.jsx
@@ +48,5 @@
>          case "incoming":
>          case "outgoing": {
>            return (<CallControllerView
>              dispatcher={this.props.dispatcher}
> +            mozLoop={this.props.mozLoop}/>);

nit: space after }

@@ +54,5 @@
>          case "room": {
>            return (<DesktopRoomConversationView
>              dispatcher={this.props.dispatcher}
>              roomStore={this.props.roomStore}
> +            mozLoop={this.props.mozLoop}/>);

Please put this above roomStore, it'll need to be there when we sort props by default.

::: browser/components/loop/content/js/roomViews.jsx
@@ +272,3 @@
>            </div>
> +          {this.props.showContext ?
> +            <DesktopRoomContextView roomStore={this.props.roomStore}/> : null}

I'd suggest passing the showContext as a prop into the view. You could even initialise the initial state of the view from the prop, and then the existing !this.state.show would check the right thing.

::: browser/components/loop/content/shared/css/conversation.css
@@ +970,5 @@
>    background-image: url("../img/icons-16x16.svg#add-active");
>  }
>  
> +.room-context {
> +  background: #000;

The spec implies this should be transparent.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +183,5 @@
>              roomToken: actionData.roomToken,
>              roomName: roomData.decryptedContext.roomName,
>              roomOwner: roomData.roomOwner,
>              roomUrl: roomData.roomUrl,
> +            roomContext: roomData.decryptedContext,

We should sync on what we're going to do here. See my patch in bug 1142588 - I'm using roomContextUrls as I thought that was clearly than just randomly storing the whole context (which would also mean we'd be duplicating the storage of roomName).
Attachment #8592196 - Flags: feedback?(standard8)
(In reply to Mark Banner (:standard8) from comment #14)
> I'd suggest passing the showContext as a prop into the view. You could even
> initialise the initial state of the view from the prop, and then the
> existing !this.state.show would check the right thing.

Basing the initial state off a prop is a React anti-pattern IIRC. Might be not, I'll look into it regardless.

> The spec implies this should be transparent.

I didn't see that TBH. I was looking at http://cl.ly/image/10431Y0u3V3r/o

> We should sync on what we're going to do here. See my patch in bug 1142588 -
> I'm using roomContextUrls as I thought that was clearly than just randomly
> storing the whole context (which would also mean we'd be duplicating the
> storage of roomName).

Just URLs won't work, because there's also the 'description' field ('comment' as shown in the mockup for the Edit context view). This is also displayed in grey italics, IF available.
Sevaan, the current spec doesn't enforce max lengths for the URL title and context comment, which makes it a bit hard to deal with text flow inside a confined space.
Can you define these, please? I'd prefer a character count for text blurbs and a definition of how to display super long URLs; ellipsis at the end, smart ellipsis insertion somewhere in the middle, only show top level domain name or something else.
Flags: needinfo?(sfranks)
This patch moves child-components away from using the ActiveRoomStoreMixin to rely on properties only.
This makes conditional rendering a bit more straightforward to follow, while still adhering to the Flux pattern.
Obviously, this needs quite a bit of work on the unit tests, so that's what I'm doing now. I just wanted to have your eyes on this sooner, rather than late.
Attachment #8592196 - Attachment is obsolete: true
Attachment #8592861 - Flags: review?(standard8)
Attachment #8592861 - Attachment is obsolete: true
Attachment #8592861 - Flags: review?(standard8)
Attachment #8593922 - Flags: review?(standard8)
Attachment #8593924 - Flags: review?(standard8)
Mark, I'm thinking about how to pass room data to the child-components here; we can do it in the way I've implemented it in the current patch, or make `DesktopRoomConversationView` pass a single object with the necessary data as properties.
I think that the benefit of the latter approach is that we'll have less property name declarations and duplication, which seems like a win to me. It'd make my work for bug 1142515 a bit easier too.
Flags: needinfo?(standard8)
Attachment #8593922 - Attachment is obsolete: true
Attachment #8593922 - Flags: review?(standard8)
Attachment #8594694 - Flags: review?(standard8)
Attachment #8593924 - Attachment is obsolete: true
Attachment #8593924 - Flags: review?(standard8)
Attachment #8594695 - Flags: review?(standard8)
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> Sevaan, the current spec doesn't enforce max lengths for the URL title and
> context comment, which makes it a bit hard to deal with text flow inside a
> confined space.

> Can you define these, please? I'd prefer a character count for text blurbs
> and a definition of how to display super long URLs; ellipsis at the end,
> smart ellipsis insertion somewhere in the middle, only show top level domain
> name or something else.

Without seeing a screenshot of how the implementation looks at the moment, it's hard to define or give a character count. Basically, though:

- Title should not be more than two lines. If it is, trail it off to ellipses at the end.
- We're only using top level domains for URLS. If the URL is long, let's trailer off to ellipses at the end.
- As for text, also, two lines worth of full-width should be the max # of characters someone can use. Let's run it full-width underneath the page thumbnail like so: http://cl.ly/image/3O2k2M3e1Z03.
Flags: needinfo?(sfranks)
Comment on attachment 8594694 [details] [diff] [review]
Patch 3.3: show context information in the conversation window

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

::: browser/components/loop/content/js/roomViews.jsx
@@ +173,5 @@
>  
>      propTypes: {
> +      dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired,
> +      error: React.PropTypes.object,
> +      roomData: React.PropTypes.object.isRequired,

This feels like a reasonable balance between passing too many items and listening to the store everywhere.

I think maybe it just needs a one-line comment explaining that its the data supplied by the activeRoomStore. Then its slightly clearer what's expected/where it comes from.

@@ +389,5 @@
> +    },
> +
> +    _shouldRenderContextView: function() {
> +      return !!(
> +        this.props.mozLoop.getLoopPref("contextInConverations.enabled") &&

I'm not sure the pref really matters here, if it does, then please fix the spelling.
Attachment #8594694 - Flags: review?(standard8) → review+
Flags: needinfo?(standard8)
Comment on attachment 8594695 [details] [diff] [review]
Patch 4.1: add tests for the context views

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

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +7,5 @@
>  
>    var ROOM_STATES = loop.store.ROOM_STATES;
>    var SCREEN_SHARE_STATES = loop.shared.utils.SCREEN_SHARE_STATES;
>  
> +  var fakeContextURL = {

nit: I think I still marginally prefer these to be initialised in the beforeEach to avoid potential future issues with tests accidentally modifying the data that other tests then think they can rely on.
Attachment #8594695 - Flags: review?(standard8) → review+
Depends on: 1158725
You need to log in before you can comment on or make changes to this bug.