1212357 - Update the layout of the rooms list items for user journey

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Mock Up of layout

As part of the user journey rework, we need to relayout the items within the rooms list. This depends on bug 1212331 for bitrot purposes.

See the user story for more detail.

Comment 1

[Manuel Casas Barrado [:mancas]]

Pau talked to me offline, about this bug and the attachment of the mock up is out of sync with the real vision of the project. Pau, could you upload the real mock up? Thank you

Comment 2

[Pau Masiá [:Pau]]

After taking a second look at it, the layout of the panel Sevaan did is the good one. Some visual tweaks for color and spacing might be needed, though.

Comment 3

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Hey Mike, could you review the patch when you get a chance?

If you think anything is wrong let me know to update the patch

Thanks!

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: edit button.png

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8674800 [details] [diff] [review]
Update the layout of the rooms list items for user journey

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

This looks quite close but I can't r+ this yet, because:

1) Clicking the pencil icon to open the dropdown yields the following JS error in the console:

TypeError: parentNode.classList is undefined
findParentNode()
 utils.js:764
ConversationDropdown<.componentDidMount()
 panel.js:547
[...snip...]

2) The favicon of the attached context is not aligned correctly with the room name.
3) The pencil icon has more right-margin than the favicon has left-margin (alignment).
4) The pencil icon is not aligned correctly for room entries that have _no_ context attached (no favicon).

::: browser/components/loop/content/js/panel.jsx
@@ +453,5 @@
>    var RoomEntryContextButtons = React.createClass({
>      propTypes: {
>        dispatcher: React.PropTypes.object.isRequired,
>        eventPosY: React.PropTypes.number.isRequired,
> +      handleContextClick: React.PropTypes.func.isRequired,

Please rename this to `handleClick`, since there are no other click handlers anymore.

::: browser/components/loop/content/shared/img/icons-10x10.svg
@@ +22,5 @@
>      use[id$="-disabled"] {
>        fill: rgba(255,255,255,0.4);
>      }
> +    use[id$="-grey"] {
> +      fill: #565656;

Since the default state (without suffix) is already a grey - #ccc - can you call this 'darkgrey' instead?

Comment 6

[Sevaan Franks [:sevaan]]

Comment on attachment 8674801 [details]
edit button.png

The edit button should be aligned to match with the right-edge of the button. http://i.sevaan.com/image/0r1J0F3x1J0L

Comment 7

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Hey Mike, I took into account all of your comments, so we can start the second round review. JFYI, I'd like to point out a couple of thoughts related to your comments:

1) It seems that we got the error there for a long time ago, but now it's fixed \o/

4) I think we should not care about the entries without context because we cannot or shouldn't create rooms with no context. However, I can modify the react component to handle no_context entries and draw a default icon instead of leaving it blank

Thanks!

Comment 8

[Manuel Casas Barrado [:mancas]]

Attachment: edit_button_context.png

Hey Sevaan, I've updated the layout, so the edit button is now aligned to the right of the available space. However, it seems that the left and right margins of the buttons are sightly different from the margins we use in the list. Please, could you confirm me, which are the correct measures?

The create room button has 1rem margin per each side and the entries have 15px.

Comment 9

[Sevaan Franks [:sevaan]]

Comment on attachment 8676711 [details]
edit_button_context.png

I think the edit button only needs to move over 2px. So let's try 13px on the right-side.

Also, the colour of the Edit icon is too light, and should match the colour of the gear icon.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Manuel Casas Barrado [:mancas] from comment #7)
> 4) I think we should not care about the entries without context because we
> cannot or shouldn't create rooms with no context. However, I can modify the
> react component to handle no_context entries and draw a default icon instead
> of leaving it blank

I think we _do_ care, because user may already have rooms without context in the version of Hello that's currently in production.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8676689 [details] [diff] [review]
Update the layout of the rooms list items for user journey

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

Well, you addressed all my comments.

The icon color will be fixed when you apply my comment below. Please re-request ui-review from Sevaan when you implemented the changes he requested.

::: browser/components/loop/content/shared/img/icons-10x10.svg
@@ +45,5 @@
>    <use id="edit" xlink:href="#edit-shape"/>
>    <use id="edit-active" xlink:href="#edit-shape"/>
>    <use id="edit-disabled" xlink:href="#edit-shape"/>
>    <use id="edit-white" xlink:href="#edit-shape"/>
> +  <use id="edit-grey" xlink:href="#edit-shape"/>

You need to rename this 'darkgrey' too :)

Comment 12

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Mike, now the panel cares about the entries with no context. Also, I've addressed the last comment so the patch should be finished. Please let me know if you think there's something wrong.

Thanks!!

Comment 13

[Manuel Casas Barrado [:mancas]]

Attachment: edit_button.png

Hey Sevaan, I've updated the margins, so every element inside the panel should be aligned.

Thanks!

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8677328 [details] [diff] [review]
Update the layout of the rooms list items for user journey

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

::: browser/components/loop/content/js/panel.jsx
@@ +351,4 @@
>  
>        return (
>          <div className="room-entry-context-item">
> +          <a href={roomUrl && roomUrl.location ? roomUrl.location : ""}

I'm not sure if you tested this, because when I start Firefox, open about:home in the active tab and click 'Start a conversation', a new room is created without a context.
Now when I hover the link I see 'chrome://browser/content/browser.xul' as the href and when I click it it opens that URL in a new tab!
I'm pretty sure you need to conditionally make this into an <a> element IF there is a URL.

Comment 15

[Sevaan Franks [:sevaan]]

Comment on attachment 8677331 [details]
edit_button.png

Looks good, thanks.

I just realized that the name of the conversation is pretty small. It should be 13pt. Can you update that in this bug or does it need a new one?

Comment 16

[Manuel Casas Barrado [:mancas]]

We can do it here since the change you mention, is related to this bug

Comment 17

[Manuel Casas Barrado [:mancas]]

Attachment: fontsizeupdated.png

Sevaan, it seems that the font-size of the conversation names is already set to 1.3rem. If you still think that is small we can change it to 1.4rem.

You can see the new size in this attachment =)

Comment 18

[Sevaan Franks [:sevaan]]

1.3 is fine.

I just realized it's not the conversation title font that is small, it's the "Recently Browsed" is too big. It should be 1.0rem I believe. http://i.sevaan.com/image/382m0S2d0e1s

Comment 19

[Manuel Casas Barrado [:mancas]]

Attachment: recentlybrowsed.png

Sevaan, the "Recently Browsed" is set to 1.1rem. I've modified it to 1rem in this screenshot. Perhaps it's to bold I think. Just let me know wdyt

Comment 20

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Hey Mike, in the meantime, could you take a look at the issue you mentioned in the previous comment?

Thanks!

Comment 21

[Sevaan Franks [:sevaan]]

Comment on attachment 8678886 [details]
recentlybrowsed.png

+r for this, but unbolded. Reviewed a screenshot over IRC with Manu already.

Comment 22

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Updating patch with css changes

Comment 23

[Ed Lee :Mardak]

Comment on attachment 8678902 [details] [diff] [review]
Update the layout of the rooms list items for user journey

>+      // XXX Fix String
>       return (
>         <div className="rooms">
>           {this._renderNewRoomButton()}
>-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
>+          <h1>RECENTLY BROWSED</h1>
Bug 1211351 is marked fixed, so the strings are finalized. Should probably use mozL10n instead of hardcoding.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8678902 [details] [diff] [review]
Update the layout of the rooms list items for user journey

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

Awesome! r=me with comments addressed.

::: browser/components/loop/content/js/panel.jsx
@@ +339,5 @@
>  
>      handleClick: function(event) {
>        event.stopPropagation();
>        event.preventDefault();
> +      console.info(event.currentTarget.href);

whoops!

@@ +725,4 @@
>        return (
>          <div className="rooms">
>            {this._renderNewRoomButton()}
> +          <h1>RECENTLY BROWSED</h1>

Can you make this an mozL10n based string, like Ed mentioned in comment 23?

Comment 26

[Manuel Casas Barrado [:mancas]]

Attachment: Update the layout of the rooms list items for user journey

Comment 27

[Ed Lee :Mardak]

Comment on attachment 8680003 [details] [diff] [review]
Update the layout of the rooms list items for user journey

>+++ b/browser/components/loop/content/js/panel.jsx
>+      // XXX Fix String
>       return (
>         <div className="rooms">
>           {this._renderNewRoomButton()}
>-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
>+          <h1>{mozL10n.get("rooms_list_recently_browsed")}</h1>
dcritch's patch in bug 1213851 just landed adding this string. You didn't need the XXX Fix String comment anyway.

I'll rebase and land your patch.

Comment 28

[Ed Lee :Mardak]

Attachment: rebased

Rebased on  bug 1213851

Comment 29

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/c85260a0fcf3018b789037a48ec7228a43ff24b2
Bug 1212357 - Update the layout of the rooms list items for user journey. r=mikedeboer

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Bogdan Maris, Desktop QA]

We did some exploratory testing around following the acceptance criteria using Firefox 44 beta 2 across platforms (Windows 7 64-bit, Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 64-bit) and can confirm that the layout is updated as intended.

Comment 32

[Bogdan Maris, Desktop QA]

And now, removing qe+ and changing tracking flag for 44 to verified.