Closed Bug 1162905 Opened 7 years ago Closed 7 years ago

Implement layout changes for context view in the rooms list

Categories

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

defect
Points:
3

Tracking

(firefox40 verified, firefox41 verified)

VERIFIED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [context][ux bug])

Attachments

(4 files, 6 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Attached file Context Changes-1.pdf
Rank: 9
Whiteboard: [context][ux bug]
Iteration: 40.3 - 11 May → 41.1 - May 25
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8605216 - Flags: review?(standard8)
Could you please post a screenshot as well, Mike?
Flags: needinfo?(mdeboer)
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.
Attachment #8605216 - Flags: review?(standard8) → review-
Attachment #8605216 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attachment #8605260 - Flags: review?(standard8)
Attached image Screen Shot 2015-05-13 at 16.28.35.png (obsolete) —
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
Attachment #8605260 - Flags: review?(standard8)
Attached image Screen Shot 2015-05-13 at 17.01.24.png (obsolete) —
Attachment #8605263 - Attachment is obsolete: true
Attachment #8605260 - Attachment is obsolete: true
Attachment #8605277 - Flags: review?(standard8)
Attachment #8605274 - Attachment is obsolete: true
Attachment #8605277 - Attachment is obsolete: true
Attachment #8605277 - Flags: review?(standard8)
Attachment #8605288 - Flags: review?(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.
Attachment #8605288 - Flags: review?(standard8) → review+
Patch for landing, carrying over r=Standard8.
Attachment #8605288 - Attachment is obsolete: true
Attachment #8605800 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c6ef413571dd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
Attached patch Patch for auroraSplinter Review
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
Attachment #8606157 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
Attachment #8606157 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.