Closed Bug 1212357 Opened 4 years ago Closed 4 years ago

Update the layout of the rooms list items for user journey

Categories

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

defect

Tracking

(firefox44 verified)

VERIFIED FIXED
mozilla44
Iteration:
44.3 - Nov 2
Tracking Status
firefox44 --- verified

People

(Reporter: standard8, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance criteria:

- Use Hardcoded strings for any new strings for now - include an "XXX Fix String" comment
- The room list title is as per shown in the mock-up.
- The room list items have the favicon, name and "pencil" menu in that order as per the attached screenshot.
- The pencil menu is the same as the existing dropdown menu.
- The pencil menu is shown when the room list item is hovered or focussed.
- Old css/strings/images are removed.

Attachments

(6 files, 7 obsolete files)

33.63 KB, image/png
Details
13.16 KB, image/png
sevaan
: ui-review+
Details
9.97 KB, image/png
Details
9.12 KB, image/png
sevaan
: ui-review+
Details
30.82 KB, patch
Details | Diff | Splinter Review
28.25 KB, patch
Details | Diff | Splinter Review
Attached image 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.
Rank: 13
Blocks: 1212361
Assignee: nobody → b.mcb
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
Flags: needinfo?(b.pmm)
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.
Flags: needinfo?(b.pmm)
Blocks: 1214582
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!
Attachment #8674800 - Flags: review?(mdeboer)
Attached image edit button.png (obsolete) —
Attachment #8674801 - Flags: ui-review?(sfranks)
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?
Attachment #8674800 - Flags: review?(mdeboer) → feedback+
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
Attachment #8674801 - Flags: ui-review?(sfranks) → ui-review-
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!
Attachment #8674800 - Attachment is obsolete: true
Attachment #8676689 - Flags: review?(mdeboer)
Attached image edit_button_context.png (obsolete) —
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.
Attachment #8676711 - Flags: ui-review?(sfranks)
Attachment #8674801 - Attachment is obsolete: true
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.
Attachment #8676711 - Flags: ui-review?(sfranks) → ui-review-
(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 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 :)
Attachment #8676689 - Flags: review?(mdeboer) → review+
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!!
Attachment #8676689 - Attachment is obsolete: true
Attachment #8677328 - Flags: review?(mdeboer)
Attached image edit_button.png
Hey Sevaan, I've updated the margins, so every element inside the panel should be aligned.

Thanks!
Attachment #8677331 - Flags: ui-review?(sfranks)
Attachment #8676711 - Attachment is obsolete: true
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.
Attachment #8677328 - Flags: review?(mdeboer) → review-
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?
Attachment #8677331 - Flags: ui-review?(sfranks) → ui-review+
We can do it here since the change you mention, is related to this bug
Attached image 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 =)
Flags: needinfo?(sfranks)
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
Flags: needinfo?(sfranks)
Attached image 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
Flags: needinfo?(sfranks)
Hey Mike, in the meantime, could you take a look at the issue you mentioned in the previous comment?

Thanks!
Attachment #8677328 - Attachment is obsolete: true
Attachment #8678888 - Flags: review?(mdeboer)
Comment on attachment 8678886 [details]
recentlybrowsed.png

+r for this, but unbolded. Reviewed a screenshot over IRC with Manu already.
Flags: needinfo?(sfranks)
Attachment #8678886 - Flags: ui-review+
Updating patch with css changes
Attachment #8678888 - Attachment is obsolete: true
Attachment #8678888 - Flags: review?(mdeboer)
Attachment #8678902 - Flags: review?(mdeboer)
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.
Duplicate of this bug: 1218572
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?
Attachment #8678902 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
Attached patch rebasedSplinter Review
Rebased on  bug 1213851
https://hg.mozilla.org/integration/fx-team/rev/c85260a0fcf3018b789037a48ec7228a43ff24b2
Bug 1212357 - Update the layout of the rooms list items for user journey. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/c85260a0fcf3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.3 - Nov 2
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
And now, removing qe+ and changing tracking flag for 44 to verified.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.