1213851 - Change the room list to show just the active room when the user is in a room

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

As part of the user journey rework, we need to show only the active room when the user is in a room. This depends on bug 1213848 to provide the initial work, and bug 1212357 to avoid bitrot.

See the user story for more detail.

Comment 1

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8675761 [details] [diff] [review]
Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

>+++ b/browser/components/loop/content/js/panel.jsx
>           <div className="room-list">{
>             this.state.rooms.map(function(room, i) {
>+              if(this.state.openedRoom !== null &&
nit: space after if

These items from the acceptance criteria don't seem to be implemented:

- When a room is open, the room list changes to "Currently Browsing"
- The room list shows only the active room
- The room has no hover effects/cannot be edited
- Add XXX comment to hard-coded strings and update bug 1213844 with their location.      

Also, it would be good to have a unit test to make sure only one active room is listed.

Comment 3

[David Critchley (:dcritch)]

Attachment: Screenshot of Room List with globe background image

Hey Sevaan/Pau,
I didn't notice the globe background image on the Rooms list entries in your mocks, are we removing this for this bug? or is this slated for another bug?

Comment 4

[Sevaan Franks [:sevaan]]

The favicon is moving over to replace those Hello icons. There is no active room indicator except the styling of the text: http://i.sevaan.com/image/333K3k1m2G35

Comment 5

[Mark Banner (:standard8)]

The "globe" was the fallback image in case of there being no favicon on a page. We should keep that as a fallback.

Comment 6

[Sevaan Franks [:sevaan]]

Yes, definitely. We use it now in the Awesome Bar instead of a dotted box, which is great.

Comment 7

[Ed Lee :Mardak]

Strings almost finalized from bug 1211351 comment 17:

- Panel labels: "Recently browsed", "Currently browsing"

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP Display only active room when user enters room

Comment 9

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8678332 [details] [diff] [review]
Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

>+++ b/browser/components/loop/content/css/panel.css
> .rooms > h1 {
>-  font-size: 1.1rem;
>+  font-size: 0.9rem;
This seems to already be fixed in bug 1212357 comment 17 that focuses on updating the layout. This bug is just to show the active room.

> .room-list > .room-entry > h2 {
>   color: #000;
> .room-list > .room-entry.room-active > h2 {
>   color: #000;
>+.room-list > .room-entry.room-opened > h2 {
>+  color: #000;
Does this color: #000 need to be here? It looks like it's always black.

>+.room-list > .room-opened:hover {
>+  background: none;
>+.room-list > .room-opened:hover > p > a:hover {
>+  opacity: .5;
>+  text-decoration: none;
Instead of overriding the styling, you should just have the styling not apply in the first place when it's :not(.room-opened).

>+++ b/browser/components/loop/content/js/panel.jsx
>-  var Checkbox = sharedViews.Checkbox;
Generally safer to not touch unrelated code and dedicate a patch to fixing things up.

>   /**
>    * Room list entry.
>+   *
>+   * Note:
>+   * There is a difference between Active Room and Opened Room...
>+   * Active Room means there are participants in the room, however,
>+   * this does not mean the user is in that room. There, also, can be
>+   * multiple Active Rooms in the list. All Active Rooms in the list
>+   * will appear in bold text.
>+   * Opened Room means the user is in the room, the room list will
>+   * only display the Opened Room, no other rooms will appear.
Doesn't seem necessary to point out there's a difference. Just describing what's and active room and an open room is enough. Probably shouldn't have the comment about how the room is styled.

Could probably be shortened too: Active Room means there are participants (the owner or others) in the room.

>   var RoomEntry = React.createClass({
>     propTypes: {
>+      isOpenedRoom: React.PropTypes.bool.isRequired,
>       room: React.PropTypes.instanceOf(loop.store.Room).isRequired
Ideally isOpenedRoom wouldn't need to be passed in as it theoretically should be something the Room knows about already. This is fine for now.


>     handleClickEntry: function(event) {
>       event.preventDefault();
>+      if (!this.props.isOpenedRoom) {
This method does nothing if it's not opened, so might as well not add the handler in the first place if the room is opened and this method can be left untouched.

>+          <h1>
>+            {
>+              /* XXX Bug 1213844 to replace these with final strings. */
>+              this.state.openedRoom === null ?
>+                "RECENTLY BROWSED" :
>+                "CURRENTLY BROWSING"
Bug 1211351 is marked fixed, so the strings are finalized. Use those instead of hardcoding.

>+++ b/browser/components/loop/modules/MozLoopService.jsm
>         window.addEventListener("unload", socialFrameChanged.bind(null, "Loop:ChatWindowClosed"));
>+        window.addEventListener("unload", windowCloseCallback);
Is this necessary?

>+++ b/browser/components/loop/test/desktop-local/panel_test.js

>+    roomData = {
>+            roomToken: "QzBbvGmIZWU",
>+            roomUrl: "http://sample/QzBbvGmIZWU",
>+            decryptedContext: {
>+              roomName: roomName
>+            },
>+          maxSize: 2,
>+            participants: [{
>+            displayName: "Alexis",
>+              account: "[e-mail]",
>+              roomConnectionId: "2a1787a6-4a73-43b5-ae3e-906ec1e763cb"
>+          }, {
>+            displayName: "Adam",
>+            roomConnectionId: "781f012b-f1ea-4ce1-9105-7cfc36fb4ec7"
>+        }],
>+      ctime: 1405517418
nit: indentation doesn't seem right here. roomData2 looks better.

>+++ b/browser/components/loop/ui/fake-mozLoop.js
>     getLoopPref: function(pref) {
>       switch (pref) {
>         // Ensure we skip FTE completely.
>         case "gettingStarted.seen":
>-          return true;
>       }
>-      return null;
Should these be removed?

Comment 11

[Ed Lee :Mardak]

You had questions?

Comment 12

[Ed Lee :Mardak]

mancas, a heads up that you're touching the strings in bug 1212357 that dcritch is also touching. The RECENTLY BROWSED could probably go in that bug and CURRENTLY BROWSING in this bug.

Although just as well if dcritch lands this with both the RECENTLY+CURRENTLY strings if it's done first.

Comment 13

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8679055 [details] [diff] [review]
Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

>+++ b/browser/components/loop/content/css/panel.css
>-.room-list > .room-entry:hover {
>+.room-list > .room-entry:not(.room-opened):hover {
>   background: #dbf7ff;
Nice.

> .room-list > .room-entry.room-active > h2 {
>   font-weight: bold;
>+.room-list > .room-entry.room-opened > h2 {
>+  font-weight: normal;
My comment about using :not(.room-opened) applies here too. No need to override the font-weight if we don't apply font-weight in the first place.

>+.room-list > .room-opened:hover > p > a:hover {
>+  opacity: .5;
>+  text-decoration: none;
What is this actually changing? I can't seem to find a state that touches this. If it's not used, it and related css should be removed instead of adding additional unused rules.

>+++ b/browser/components/loop/content/js/panel.jsx
>   /**
>    * Room list entry.
>+   *
>+   * Note:
"Note:" seems unnecessary
>+   * Active Room means there are participants in the room.
>+   * Opened Room means the user is in the room.

>     render: function() {
>+      var handleClickEntryFunc =
>+        this.props.isOpenedRoom ? function() {} : this.handleClickEntry;
>+      var handleMouseOutFunc =
>+        this.props.isOpenedRoom ? function() {} : this._handleMouseOut;
These can be placed inline. And you can use "null" instead of "function() {}", see below.

>       return (
>         <div className={roomClasses}
>-          onClick={this.handleClickEntry}
>-          onMouseLeave={this._handleMouseOut}
+            onClick={this.props.isOpenedRoom ? null : this.handleClickEntry}
+            onMouseLeave={this.props.isOpenedRoom ? null : this._handleMouseOut}

>-          <RoomEntryContextButtons
>-            handleClickEntry={this.handleClickEntry}
>+          {this.props.isOpenedRoom ? null :
>+            <RoomEntryContextButtons
>+              handleClickEntry={handleClickEntryFunc}
You can just use the original this.handleClickEntry because this only exists when isOpenedRoom is null anyway.

>-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
>+          <h1>
>+            {
>+              /* XXX Bug 1213844 to replace these with final strings. */
>+              this.state.openedRoom === null ?
>+                mozL10n.get("rooms_list_recently_browsed") :
>+                mozL10n.get("rooms_list_currently_browsing")
The comment is obsolete.

>+++ b/browser/components/loop/test/desktop-local/panel_test.js
>+    it("should only show the active room you're in when you're in a room", function() {
The test should probably reference "opened room"

>+      roomStore.setStoreState({ rooms: roomList, openedRoom: roomList[0].roomToken });
>+
>+      var view = createTestComponent();
>+
>+      var node = view.getDOMNode();
>+      expect(node.querySelectorAll(".room-opened").length).to.eql(1);
>+      expect(node.querySelectorAll(".room-entry").length).to.eql(1);
>+      expect(node.querySelectorAll(".room-opened h2")[0].textContent).to.equal(roomName);
>+    });
Would be good to add a test making sure OpenRoom action wasn't dispatched on click of an opened room.

Comment 15

[Ed Lee :Mardak]

Attachment: hover opened video

One thing I noticed when testing this, for an opened room that's not hovered, it shows the globe where the dropdown would appear. But when you hover the entry, the globe disappears. Should the globe be disappearing?

Comment 16

[Sevaan Franks [:sevaan]]

Comment on attachment 8679076 [details]
hover opened video

The globe shouldn't disappear, no. Also, it should be replacing the Hello icon on the left. I think dcritch is working on this though.

Comment 17

[David Critchley (:dcritch)]

(In reply to Sevaan Franks [:sevaan] from comment #16)
> Comment on attachment 8679076 [details]
> hover opened video
> 
> The globe shouldn't disappear, no. Also, it should be replacing the Hello
> icon on the left. I think dcritch is working on this though.

I didn't think replacing the hello icon was part of this bug.  Also, I couldn't find that globe in the DOM that I'm working in?

Comment 18

[Ed Lee :Mardak]

The globe comes from the context:

https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/content/js/panel.jsx#356

Comment 19

[David Critchley (:dcritch)]

(In reply to Sevaan Franks [:sevaan] from comment #16)
> Comment on attachment 8679076 [details]
> hover opened video
> 
> The globe shouldn't disappear, no. Also, it should be replacing the Hello
> icon on the left. I think dcritch is working on this though.

The globe disappears to allow a place for the buttons. I found some css that give the globe a display:none when the user hovers over the room-entry.  If I remove this, the buttons wrap to the next line. I believe this display:none should be removed as part of this other bug.

Comment 20

[Ed Lee :Mardak]

(In reply to David Critchley (:dcritch) from comment #19)
> The globe disappears to allow a place for the buttons. I found some css that
> give the globe a display:none when the user hovers over the room-entry. 
> If I remove this, the buttons wrap to the next line.
Clearly, that's the wrong fix then. Your patch doesn't show buttons for the opened room, so there's space to show the icon.

The fix should be just changing the css selector like you did for the other parts of the patch.

Comment 21

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 22

[Ed Lee :Mardak]

Comment on attachment 8679142 [details] [diff] [review]
Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Normally r+ means you don't need to request review again. But you didn't answer my question from the other review comments:

>+.room-list > .room-opened:hover > p > a:hover {
>+  opacity: .5;
>+  text-decoration: none;
What is this actually changing? I can't seem to find a state that touches this. If it's not used, it and related css should be removed instead of adding additional unused rules.

>+++ b/browser/components/loop/content/js/panel.jsx
>+      var handleClickEntryFunc = this.props.isOpenedRoom ? null : this.handleClickEntry;
>+      var handleMouseOutFunc = this.props.isOpenedRoom ? null : this._handleMouseOut;
> 
>       return (
>         <div className={roomClasses}
>-          onClick={this.handleClickEntry}
>-          onMouseLeave={this._handleMouseOut}
>+          onClick={handleClickEntryFunc}
>+          onMouseLeave={handleMouseOutFunc}
When I said inline, I meant you don't need to declare a handleClickEntryFunc then use it. Just put the tertiary in the jsx {}.

>-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
>+          <h1>
>+            {
>+              this.state.openedRoom === null ?
>+                mozL10n.get("rooms_list_recently_browsed") :
>+                mozL10n.get("rooms_list_currently_browsing")
>+            }
>+          </h1>
This wrapping doesn't look like anything else in the code. Probably should have something more like <h1>{mozL10n.get(opened ? "browsed" : "browsing")}</h1>.

>+++ b/browser/components/loop/ui/fake-mozLoop.js
>@@ -114,17 +114,17 @@ var fakeRooms = [
>     getLoopPref: function(pref) {
>-      return null;
>+      return false;
Is this change necessary?

Comment 23

[David Critchley (:dcritch)]

(In reply to Ed Lee :Mardak from comment #22)
> Comment on attachment 8679142 [details] [diff] [review]
> Attachment to Bug 1213851 - Change the room list to show just the active
> room when the user is in a room
> 
> Normally r+ means you don't need to request review again. But you didn't
> answer my question from the other review comments:
> 
> >+.room-list > .room-opened:hover > p > a:hover {
> >+  opacity: .5;
> >+  text-decoration: none;
> What is this actually changing? I can't seem to find a state that touches
> this. If it's not used, it and related css should be removed instead of
> adding additional unused rules.
> 
> >+++ b/browser/components/loop/content/js/panel.jsx
> >+      var handleClickEntryFunc = this.props.isOpenedRoom ? null : this.handleClickEntry;
> >+      var handleMouseOutFunc = this.props.isOpenedRoom ? null : this._handleMouseOut;
> > 
> >       return (
> >         <div className={roomClasses}
> >-          onClick={this.handleClickEntry}
> >-          onMouseLeave={this._handleMouseOut}
> >+          onClick={handleClickEntryFunc}
> >+          onMouseLeave={handleMouseOutFunc}
> When I said inline, I meant you don't need to declare a handleClickEntryFunc
> then use it. Just put the tertiary in the jsx {}.
> 
> >-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
> >+          <h1>
> >+            {
> >+              this.state.openedRoom === null ?
> >+                mozL10n.get("rooms_list_recently_browsed") :
> >+                mozL10n.get("rooms_list_currently_browsing")
> >+            }
> >+          </h1>
> This wrapping doesn't look like anything else in the code. Probably should
> have something more like <h1>{mozL10n.get(opened ? "browsed" :
> "browsing")}</h1>.
> 
> >+++ b/browser/components/loop/ui/fake-mozLoop.js
> >@@ -114,17 +114,17 @@ var fakeRooms = [
> >     getLoopPref: function(pref) {
> >-      return null;
> >+      return false;
> Is this change necessary?

Some of this was a result of several merge conflicts that messed up my local repo. So I was working through that , obviously I missed some. But fixed now, thanks for catching.
As for answering your question as to what it is changing... I'm not sure, I couldn't find what it was changing. I may have added that, or it may have been, again, a result of the messed up rebase. In any case, I removed it and tested and it appears to be working fine.

Comment 24

[Ed Lee :Mardak]

Well, if you changed it and didn't see any changes, it might be dead css rules. We can clean that up in a followup bug.

Comment 25

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 26

[Ed Lee :Mardak]

Comment on attachment 8679548 [details] [diff] [review]
Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

>+++ b/browser/components/loop/content/js/panel.jsx
>-          <h1>{mozL10n.get("rooms_list_recent_conversations")}</h1>
>+          <h1>
>+            {
>+              mozL10n.get(this.state.openedRoom === null ?
>+                "rooms_list_recently_browsed" :
>+                "rooms_list_currently_browsing")
>+            }
>+          </h1>
This still doesn't look like anything else in the codebase. My suggestion was to make it start on the same line as the <h1> just like it was before your changes.

Comment 27

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 28

[Carsten Book [:Tomcat]]

this failed to apply:

patching file browser/components/loop/content/js/panel.js
Hunk #2 FAILED at 416
1 out of 4 hunks FAILED -- saving rejects to file browser/components/loop/content/js/panel.js.rej
patching file browser/components/loop/content/js/panel.jsx
Hunk #2 FAILED at 416
1 out of 4 hunks FAILED -- saving rejects to file browser/components/loop/content/js/panel.jsx.rej
patching file browser/components/loop/test/desktop-local/panel_test.js
Hunk #8 FAILED at 784
1 out of 10 hunks FAILED -- saving rejects to file browser/components/loop/test/desktop-local/panel_test.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1213851---Display-only-active-room-when-user-e.patch

could you take a look, thanks!

Comment 29

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1213851 - Change the room list to show just the active room when the user is in a room

Display only active room when user enters room

Comment 30

[David Critchley (:dcritch)]

Rebased and pushed up

Comment 31

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/80a9d5e49dc61ab479f988fbb6dfa3864a4ee0ab
Bug 1213851 - Display only active room when user enters room [r=Mardak]

Comment 32

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/8486fdc26fa6a41a845ea5efc72953b539988607
Bug 1213851 - test bustage fix when rebasing on bug 1214582 [r=Mardak]

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/80a9d5e49dc6
https://hg.mozilla.org/mozilla-central/rev/8486fdc26fa6