Closed Bug 1162909 Opened 9 years ago Closed 9 years ago

Implement layout changes for context view in the conversation window

Categories

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

defect
Points:
5

Tracking

(firefox40 verified, firefox41 verified)

VERIFIED FIXED
mozilla41
Iteration:
41.2 - Jun 8
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [context])

Attachments

(5 files, 2 obsolete files)

Inside the conversation window we allow attaching a context to a new conversation/ room and editing context data.
Sevaan has provided some layout updates (see attachment) to this view inside the conversation window, which we should implement.
Flags: qe-verify+
Flags: firefox-backlog+
Attached file Context Changes-1.pdf
Attachment #8603288 - Flags: review?(standard8) → review+
Keywords: leave-open
Whiteboard: [context]
Rank: 9
Iteration: 40.3 - 11 May → 41.1 - May 25
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8607596 - Flags: ui-review?(sfranks)
Attachment #8607595 - Flags: ui-review?(sfranks)
Comment on attachment 8607596 [details]
Screen Shot 2015-05-19 at 18.05.19.png

+r, but I would suggest making the spacing between everything a little tighter (10px spacing) as it will look a lot slicker: http://i.sevaan.com/image/3T1D0w22190b
Attachment #8607596 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8607595 [details]
Screen Shot 2015-05-19 at 18.05.09.png

Hmm. +r, but I wonder about the dark favicon on the dark background. It's not a big deal in the future as the refresh doesn't have a dark background, but right now favicons could get lost. I would suggest a white box with rounded corners behind it ala: http://i.sevaan.com/image/3O3O2Y2E0e43 That way it will stand out no matter what.

Additionally, if the link icon and text could be moved up a little more, it would mean we could make that whole context area a little shorter (shorter height not shown in the mockup)
Attachment #8607595 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8608116 - Flags: review?(standard8)
Comment on attachment 8608116 [details] [diff] [review]
Patch 2: update the context views with latest UX updates

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

Ok, I think this is close, but there's one question, and a couple more tests that I think need adding.

I think there's still a few things we'll need to fix following this, but lets get this improvement landed and we'll go through them with Sevaan.

::: browser/components/loop/modules/LoopRooms.jsm
@@ +731,5 @@
>          roomName: roomData.roomName || room.roomName
>        };
> +    }
> +    // Make sure to _always_ encrypt the roomName.
> +    if (!room.decryptedContext.roomName && (roomData.roomName || room.roomName)) {

So this bit has bitrotted, but I don't understand the change and the comment.

This is now an if rather than an else, so that's more restrictive than before, when we were assigning the room name regardless of any change.

The room already exists, so in theory it will always have a room.decryptedContext.roomName.

Were you trying to get this so that if a room name isn't specified, then it doesn't change the room name?

If so, I think you want to make the change to something like:

else if (roomData.roomName || room.roomName).

Though, why aren't we just sending the existing room name?

I think once we know it'd be worth extending the tests for this if we're keeping a change here.

::: browser/components/loop/test/desktop-local/roomStore_test.js
@@ +643,5 @@
>          roomName: "silly name"
>        });
>      });
>  
> +    it("should flag the the store as saving context", function() {

Can you extend the should store any update-encountered error test to check the savingContext gets reset to false?

Also, it'd be good to have a test for the case where no properties have changed, and savingContext should be reset.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +782,5 @@
> +          expect(node.querySelector(".checkbox-wrapper").classList.contains("hide")).to.eql(true);
> +
> +          next();
> +        });
> +      })

eslint says missing semi-colon.
Attachment #8608116 - Flags: review?(standard8) → review-
(In reply to Mark Banner (:standard8) from comment #12)
> So this bit has bitrotted, but I don't understand the change and the comment.
> 
> This is now an if rather than an else, so that's more restrictive than
> before, when we were assigning the room name regardless of any change.
> 
> The room already exists, so in theory it will always have a
> room.decryptedContext.roomName.
> 
> Were you trying to get this so that if a room name isn't specified, then it
> doesn't change the room name?
> 
> If so, I think you want to make the change to something like:
> 
> else if (roomData.roomName || room.roomName).
> 
> Though, why aren't we just sending the existing room name?
> 
> I think once we know it'd be worth extending the tests for this if we're
> keeping a change here.

So your changes to this function actually fixed the issue that the room name was reset to "" when none was supplied as a changed property. I removed the changes here, so the end result is nil.
Since we already had tests that covered to roomStore room update cases, I thought it'd make sense to fold the `savingContext` assertions alongside them, instead of splitting them off to separate assertions.
Attachment #8608116 - Attachment is obsolete: true
Attachment #8610567 - Flags: review?(standard8)
Attachment #8610567 - Attachment is obsolete: true
Attachment #8610567 - Flags: review?(standard8)
Attachment #8611180 - Flags: review?(standard8)
Iteration: 41.1 - May 25 → 41.2 - Jun 8
Comment on attachment 8610567 [details] [diff] [review]
Patch 2.1: update the context views with latest UX updates

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

Ok, this looks good now. Just a couple of minor comments that I think we need to handle the rtl mode. If you want to re-request review for those, please do, otherwise r=Standard8

::: browser/components/loop/content/shared/css/conversation.css
@@ +1157,4 @@
>  .room-context-btn-close,
>  .room-context-btn-edit {
>    position: absolute;
> +  right: 8px;

So I think this should have handling for rtl mode.

@@ +1171,5 @@
>    cursor: pointer;
>  }
>  
>  .room-context-btn-edit {
> +  right: 20px;

Ditto wrt rtl mode.
Attachment #8610567 - Attachment is obsolete: false
Comment on attachment 8611180 [details] [diff] [review]
Patch 2.2: update the context views with latest UX updates

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

Ok, I really meant to do give r+ on this one ;-)

The rtl comments still apply though.
Attachment #8611180 - Flags: review?(standard8) → review+
I'm pretty sure this wasn't meant to be leave-open still.
Keywords: leave-open
(In reply to Mark Banner (:standard8) from comment #24)
> I'm pretty sure this wasn't meant to be leave-open still.

Should this get marked as fixed then?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8610567 - Attachment is obsolete: true
Comment on attachment 8611180 [details] [diff] [review]
Patch 2.2: update the context views with latest UX updates

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: This patch implements various improvements to the UX of this new Hello feature inside the conversation window, making it much more usable as a result.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.
Attachment #8611180 - Flags: approval-mozilla-aurora?
QA Contact: bogdan.maris
Any reason why this is not riding the train?

The patch is pretty big.
(In reply to Sylvestre Ledru [:sylvestre] from comment #27)
> Any reason why this is not riding the train?
> 
> The patch is pretty big.

Well, we wouldn't like to have the context in conversations feature riding the trains to release in its current state. Sevaan and others from the UX team reviewed the current state as it is on Fx 40 and this patch is the result of incorporating their change requests. Fixes include remedies for serious usability flaws.
The largest number of LOC are used for cosmetic changes; indentation, markup placement and CSS. Hence the low risk assessment in comment 26.

So the current state of affairs is that, unfortunately, part of the development of this feature leaked into the 41 development cycle.
However, I think it's more work now to back-out the feature entirely than to land this patch.
Flags: needinfo?(sledru)
Comment on attachment 8611180 [details] [diff] [review]
Patch 2.2: update the context views with latest UX updates

OK, let's take it. We have time to fix potential regressions.
Flags: needinfo?(sledru)
Attachment #8611180 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs rebasing for Aurora uplift. Might as well fold in the follow-up fix while you're at it.
Flags: needinfo?(mdeboer)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30)
> This needs rebasing for Aurora uplift. Might as well fold in the follow-up
> fix while you're at it.

Will do ;-)
Flags: needinfo?(mdeboer)
Depends on: 1171925
Depends on: 1172847
Depends on: 1172910
Depends on: 1172915
Depends on: 1174702
I performed extended exploratory testing around this layout changes using latest Aurora and Nightly across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and found a few issues that were added to dependencies. Marking this bug as verified fixed based on my testing, the remaining issues will be resolved in their own bugs.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1175841
You need to log in before you can comment on or make changes to this bug.