1183638 - Implement the refreshed design for the conversation list

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

As the acceptance criteria in bug 1179193 states:

 - Conversation list refresh (indicator of someone in conversation and list layout)

Note: this _does_ include the individual room entries; removing the 'Copy' and 'Delete' buttons, replacing them with the green call button.

For the visual design spec, please check out the ones attached to bug 1179193.

Comment 1

[Mark Banner (:standard8)]

Reworking for what we think needs doing, see the user story for the detail of what needs doing.

Comment 2

[Andrei Oprea [:andreio]]

Could not find the icon in this screenshot https://www.dropbox.com/s/ecfehwsq170jxml/Screenshot%202015-08-11%2016.23.39.png?dl=0

UI review: https://www.dropbox.com/s/jg8rhos9jx44pbz/Screenshot%202015-08-11%2017.02.51.png?dl=0

Comment 3

[Andrei Oprea [:andreio]]

Attachment: Loop conversation list panel refresh

Comment 4

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #2)
> Could not find the icon in this screenshot
> https://www.dropbox.com/s/ecfehwsq170jxml/Screenshot%202015-08-11%2016.23.39.
> png?dl=0
> 
> UI review:
> https://www.dropbox.com/s/jg8rhos9jx44pbz/Screenshot%202015-08-11%2017.02.51.
> png?dl=0

There in assets export folder.

Comment 5

[Mark Banner (:standard8)]

Attachment: Icon slightly too high?

Looking at it on my screen, the icon looks like its slightly too high/tall when compared with the text.

Andrei, is this the same on yours, or are we looking at differences here?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8646673 [details] [diff] [review]
Loop conversation list panel refresh

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

A few smallish comments for some improvements that need fixing up. We also need to work out about the icon placement.

::: browser/components/loop/content/css/panel.css
@@ +346,5 @@
>  .room-list {
>    max-height: 335px; /* XXX better computation needed */
>    min-height: 7px;
>    overflow: auto;
> +  margin: 0 -1.5rem;

This is a bit strange - the whole `.rooms` section has a 1.5rem padding, we then ignore that with this -1.5rem, only to put it back in with 1.5rem on `.room-list > .room-entry`.

How about we make it simpler, remove this margin section, and just have `padding: .2 0;` in the `.room-list > .room-entry` ?

::: browser/components/loop/content/js/panel.jsx
@@ -656,5 @@
> -      var numRooms = this.state.rooms.length;
> -      if (numRooms === 0) {
> -        return mozL10n.get("rooms_list_no_current_conversations");
> -      }
> -      return mozL10n.get("rooms_list_current_conversations", {num: numRooms});

Can you check there's a bug for implementing the "no conversations" view please?

rooms_list_current_conversations and rooms_list_no_current_conversations should also be removed now so that we don't forget them.

::: browser/components/loop/ui/ui-showcase.css
@@ -93,5 @@
>    text-decoration: none;
>    color: #555;
>  }
>  
> -.showcase .checkbox-wrapper label {

iirc, this was for the RTL box in the header, how about scoping this like:

.showcase > header > .checkbox-wrapper > label

?

Could do the same for .showcase .checkbox.checked as well.

Comment 7

[Andrei Oprea [:andreio]]

(In reply to Mark Banner (:standard8) from comment #5)
> Created attachment 8647510 [details]
> Icon slightly too high?
> 
> Looking at it on my screen, the icon looks like its slightly too high/tall
> when compared with the text.
> 
> Andrei, is this the same on yours, or are we looking at differences here?

Did not look that off on my machine. Went ahead and changed the code so that we use pseudo elements and also tweaked the position some more. Here is how it looks for me.

https://www.dropbox.com/s/or5hf7yz39bgdbm/Screenshot%202015-08-13%2013.32.14.png?dl=0

Comment 8

[Andrei Oprea [:andreio]]

(In reply to Mark Banner (:standard8) from comment #6)
> Comment on attachment 8646673 [details] [diff] [review]
> Loop conversation list panel refresh
> 
> Review of attachment 8646673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few smallish comments for some improvements that need fixing up. We also
> need to work out about the icon placement.
> 
> ::: browser/components/loop/content/css/panel.css
> @@ +346,5 @@
> >  .room-list {
> >    max-height: 335px; /* XXX better computation needed */
> >    min-height: 7px;
> >    overflow: auto;
> > +  margin: 0 -1.5rem;
> 
> This is a bit strange - the whole `.rooms` section has a 1.5rem padding, we
> then ignore that with this -1.5rem, only to put it back in with 1.5rem on
> `.room-list > .room-entry`.
> 
> How about we make it simpler, remove this margin section, and just have
> `padding: .2 0;` in the `.room-list > .room-entry` ?

Was there because of needed padding between content and container & at the same time the room list entries needed to span from edge to edge (when hovered, see blue background).
Removed the weird negative margin rules and added explicit margin on each element.

> 
> ::: browser/components/loop/content/js/panel.jsx
> @@ -656,5 @@
> > -      var numRooms = this.state.rooms.length;
> > -      if (numRooms === 0) {
> > -        return mozL10n.get("rooms_list_no_current_conversations");
> > -      }
> > -      return mozL10n.get("rooms_list_current_conversations", {num: numRooms});
> 
> Can you check there's a bug for implementing the "no conversations" view
> please?
> 

Good catch. There wasn't implementing in this patch.

> rooms_list_current_conversations and rooms_list_no_current_conversations
> should also be removed now so that we don't forget them.

Done.

> 
> ::: browser/components/loop/ui/ui-showcase.css
> @@ -93,5 @@
> >    text-decoration: none;
> >    color: #555;
> >  }
> >  
> > -.showcase .checkbox-wrapper label {
> 
> iirc, this was for the RTL box in the header, how about scoping this like:
> 
> .showcase > header > .checkbox-wrapper > label
> 
> ?
> 
> Could do the same for .showcase .checkbox.checked as well.

Did not understand this part. The rule made the font bold but only in showcase and it does not match the spec so I removed it.

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Loop conversation list panel refresh

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8647918 [details] [diff] [review]
Loop conversation list panel refresh

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

Looks good. I've filed bug 1194622 on the fact we'll now flicker to no current conversations on initial start. We need to improve that UI anyway, so that's a good opportunity.

r=Standard8

Comment 11

[Andrei Oprea [:andreio]]

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

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/5c906eb9b8d9

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Verified using latest Nightly 43.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit) that the refreshed design for conversation list matches the implementation here.