1162905 - Implement layout changes for context view in the rooms list

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Inside the panel we allow attaching a context to a new conversation/ room.
Sevaan has provided some layout updates (see attachment) to this view inside the Loop panel, which we should implement.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Context Changes-1.pdf

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: UX refresh for the available context area

Comment 3

[Sevaan Franks [:sevaan]]

Could you please post a screenshot as well, Mike?

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8605216 [details] [diff] [review]
Patch v1: UX refresh for the available context area

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

I've just tried testing this on top of bug 1162903 and it seems I can't get the checkbox to toggle anymore. Nothing in the error console.

::: browser/components/loop/content/js/panel.jsx
@@ -772,5 @@
>            <div className={contextClasses}>
> -            <label className="context-enabled">
> -              <input className="context-checkbox"
> -                     type="checkbox" onChange={this.onCheckboxChange}/>
> -              {mozL10n.get("context_offer_label")}

We should remove the string for this from the properties file. We might need a separate patch for uplift, but that should be easy.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: UX refresh for the available context area

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Screen Shot 2015-05-13 at 16.28.35.png

Comment 7

[Sevaan Franks [:sevaan]]

Thanks. The text and URL should all be aligned together. I would move it over to the right a couple pixels as well to match the spacing from the left-edge of the box to the left-edge of the favicon.

Additionally, it should only be capturing the page title, not any text from the page itself (not sure if that's a page title in the mockup.)

http://i.sevaan.com/image/3V1T3n0O0x03

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Screen Shot 2015-05-13 at 17.01.24.png

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: UX refresh for the available context area

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Screen Shot 2015-05-13 at 17.16.03.png

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3: UX refresh for the available context area

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8605288 [details] [diff] [review]
Patch v1.3: UX refresh for the available context area

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

One small nit, otherwise looks great.

::: browser/components/loop/content/js/roomViews.jsx
@@ +312,5 @@
>          // checkbox will be disabled in that case.
>          if (nextProps.editMode) {
>            this.props.mozLoop.getSelectedTabMetadata(function(metadata) {
>              var previewImage = metadata.favicon || "";
> +            var description = metadata.title || metadata.description;

Did you mean to make this change? If so, you forgot the roomViews.js update.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4: UX refresh for the available context area

Patch for landing, carrying over r=Standard8.

Comment 14

[Pulsebot]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6ef413571dd

Comment 16

[Mark Banner (:standard8)]

Attachment: Patch for aurora

Patch for aurora without the string removal.

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: Improves the layout of the context details in the panel to make it more appealing.
[Describe test coverage new/current, TreeHerder]: Has unit tests
[Risks and why]: Minor, only affects loop specific code
[String/UUID change made/needed]: None

Comment 17

[Bogdan Maris, Desktop Test Engineering]

UX looks fine, as shown in mockup, using latest Nightly, across platforms (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5). Will mark this bug as verified fixed but keeping qe-verify+ to retest in Aurora.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24b18a77678a

Comment 19

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #17)
> UX looks fine, as shown in mockup, using latest Nightly, across platforms
> (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5). Will mark this
> bug as verified fixed but keeping qe-verify+ to retest in Aurora.

Also verified using latest Aurora across platforms (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5).