1074672 - Implement a general Room list view in Loop

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

[Mark Banner (:standard8)]

(Commenting on User Story)
> * Rooms are listed in chronological order of activity (most recently active
> at the top). An activity is "room created" or "someone joined a room"

Romain: We could possibly based this on listing rooms in reverse order of latest expiry time, which, combined with bug 1074700 might give you what you want.

If you want anything different, then we'll need to move it to a separate bug as we don't have anywhere we store the information at the moment, so we'll have to work out the requirements and where we will store it etc.

Comment 2

[Romain Testard [:RT]]

(In reply to Mark Banner (:standard8) from comment #1)
> (Commenting on User Story)
> > * Rooms are listed in chronological order of activity (most recently active
> > at the top). An activity is "room created" or "someone joined a room"
> 
> Romain: We could possibly based this on listing rooms in reverse order of
> latest expiry time, which, combined with bug 1074700 might give you what you
> want.
Yes this gives what we need, I'm updating the user story so it is clearer.
> 
> If you want anything different, then we'll need to move it to a separate bug
> as we don't have anywhere we store the information at the moment, so we'll
> have to work out the requirements and where we will store it etc.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Mark Banner (:standard8) from comment #1)
> Romain: We could possibly based this on listing rooms in reverse order of
> latest expiry time, which, combined with bug 1074700 might give you what you
> want.

Reading https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms, I think we could just sort the room list by ctime DESC; FTR ctime is:

>ctime - Similar in spirit to the Unix filesystem "ctime" (change time) attribute. The time, in seconds >since the Unix epoch, that any of the following happened to the room:
>
>    The room was created
>    The owner modified its attributes with "PATCH /rooms/{token}"
>    A user joined the room
>    A user left the room

Also, ctime is available as record properties in the list, while expiry time isn't.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop (WiP)

This is for getting early feedback about the initial implementation of a room store. No tests yet, consider this as a sandbox.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop.

New experiment, this time I'm not extending Backbone.Collection to ease dealing with songle room operations; instead, the store simply extends Backbone.Events so it can notify, and maps collection events.

I have a feeling we could get rid of Backbone.Collection (and model) entirely here. Feedback and thoughts welcome.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8499556 [details] [diff] [review]
Implement a room list view for Loop.

Instead of attaching a patch for each interation of my investigation, let me just link to the always up to date diff on github: Switching to linking to a github branch for feedback: https://github.com/n1k0/gecko-dev/compare/mozilla:fx-team...bug-1074672-room-list-view

Feel free to comment here or there.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

The first part of the patch is now ready for review. Please note it adds a new `loop.rooms.enabled` pref, which is set to false by default. You obviously need to turn it on to see the new Rooms tab, and the list.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

Sorry, forgot to commit some stuff. Message is still:

The first part of the patch is now ready for review. Please note it adds a new `loop.rooms.enabled` pref, which is set to false by default. You obviously need to turn it on to see the new Rooms tab, and the list.

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

Following discussion on IRC, there's now a dedicated store for room list (roomStore.js has been renamed to roomListStore.js).

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8500411 [details] [diff] [review]
Implement a room list view for Loop - Part 1

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

NiKo: These are comments on this first version, that were made before you updated it, so some may no longer be relevant.

::: browser/components/loop/content/js/panel.jsx
@@ +439,5 @@
> +   */
> +  var RoomEntry = React.createClass({
> +    propTypes: {
> +      openRoom: React.PropTypes.func.isRequired,
> +      room:     React.PropTypes.object.isRequired

It would be nice to limit this to a specific type, if possible.

@@ +452,5 @@
> +      this.props.openRoom(this.props.room);
> +    },
> +
> +    displayRoomUrl: function() {
> +      return "http://XXXchangeme/" + this.props.room.roomToken;

I've just filed bug 1078311 to allow the rooms list to return the roomUrl. This will make life far easier for us.

@@ +582,5 @@
>        this.updateServiceErrors();
>      },
>  
> +    /**
> +     * The rooms feature is hidden by default for now. Once they get mainstream,

nit: Once it gets mainstream

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +272,5 @@
>  ## LOCALIZATION NOTE (feedback_report_user_button): Used to report a user in the case of
>  ## an abusive user.
>  feedback_report_user_button=Report User
> +
> +rooms_list_current_conversations=Current conversations

The spec gives us the option for "No current conversations" if there's zero in the list.

Although that might be ok as a separate string, I had a discussion with L10n folks and we came to the conclusion that we should do this as a plural string - for the few locales where a "No current..." or "Current conv" doesn't quite fit.

We should add a note to the effect of: We prefer to have no number in the string, but if you need it for your language please use {{number}}

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop.

Addressed review comments. There's a problem with the plural form handling; I've added a string in that form:

rooms_list_current_conversations=No current conversation;Current conversations

and translation retrieval is done that way in the code:

mozL10n.get("rooms_list_current_conversations", {num: 42})

The thing is, with num === 0, I get "No current conversation" as expected. But with n > 0, I get "No current conversation;Current conversations" (the whole plural form source string). I suspect this is a bug, but I'm not 100% sure, so input welcome.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop.

Some fixes. Previous comment about l10n remains.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8500646 [details] [diff] [review]
Implement a room list view for Loop.

I'm out tomorrow, so Dan or Mike might be better to review this.

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

Patch rebased on top of latest fx-team.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8500943 [details] [diff] [review]
Implement a room list view for Loop - Part 1

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

Lots of new stuff! I think the next patch should be very close to being ready!

I didn't see the updated styles for the tabs (the active tab now has a blue bar at its bottom), are you also going to address that in this bug?

::: browser/components/loop/content/js/panel.jsx
@@ +23,5 @@
>  
>    var TabView = React.createClass({
>      getInitialState: function() {
>        return {
> +        selectedTab: this.props.selectedTab || "call"

If you're using this prop only for tests, could you mention that here in a comment?

@@ +473,5 @@
> +          <span className={pillClasses} />
> +          <h2>{room.roomName}</h2>
> +          <p>
> +            <a ref="room" href="#" onClick={this.handleClickRoom}>
> +              {this.props.room.roomUrl}

this can be `room.roomUrl`

@@ +512,5 @@
> +      this.setState({rooms: this.props.store.getStoreState()});
> +    },
> +
> +    openRoom: function(room) {
> +      // XXX implement me

please add a bug number here

::: browser/components/loop/content/shared/css/panel.css
@@ +127,5 @@
> +
> +.room-list > h1 {
> +  font-weight: bold;
> +  color: #999;
> +  padding: .5rem 1rem;

What's the reasoning behind going with rem units for rooms specifically?

::: browser/components/loop/content/shared/js/actions.js
@@ +123,5 @@
> +    }),
> +
> +    /**
> +     * Retrieves room list.
> +     * XXX: should move to some roomActions module.

Got a bug number handy for that?

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +4,5 @@
>  
>  /* global loop:true */
>  
>  var loop = loop || {};
> +loop.store = loop.store || {};

If you refactor a patch like this, it's very hard for the reviewer to see what's going on... additionally I don't really see what we're gaining with this new way of writing things.

This introduces two new globals: `WS_STATES` and `CALL_STATES`. Is that intentional?

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +6,5 @@
> +
> +var loop = loop || {};
> +loop.store = loop.store || {};
> +
> +loop.store.Room = (function() {

I don't see why you'd need two closures in one JS file...

```js
var loop = loop || {};
loop.store = loop.store || {};

(function() {

var roomSchema = {
 ...
};

loop.store.Room = Room;

...

loop.store.RoomListStore = RoomListStore;

})();
```

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +181,5 @@
> +        });
> +
> +        it("should select contacts tab when clicking tab button", function() {
> +          TestUtils.Simulate.click(
> +            view.getDOMNode().querySelector('li[data-tab-name="contacts"]'));

I don't think that having double-quotes in a string warrant the use of single quotes. Please use escape characters like `\"`

::: browser/components/loop/test/shared/roomListStore_test.js
@@ +61,5 @@
> +    it("should fetch the room list from the mozLoop API", function() {
> +      dispatcher.dispatch(new sharedActions.GetAllRooms());
> +
> +      store.once("change", function() {
> +        // XXX: this asserts on sample fixtures for now!

Won't the test always use sample fixtures?

::: browser/components/loop/ui/fake-mozLoop.js
@@ +9,5 @@
>  navigator.mozLoop = {
>    ensureRegistered: function() {},
>    getLoopCharPref: function() {},
> +  getLoopBoolPref: function(pref) {
> +    // ensure UI for rooms is displayed in the showcase

nit: plz capitalize and end your comment with a dot

@@ +10,5 @@
>    ensureRegistered: function() {},
>    getLoopCharPref: function() {},
> +  getLoopBoolPref: function(pref) {
> +    // ensure UI for rooms is displayed in the showcase
> +    if (pref === "rooms.enabled") {

nit: not sure why you're using strict equality comparison here, but I'm not willing to get into a mud pool and wrestle about it... moving on!

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +274,5 @@
>  feedback_report_user_button=Report User
> +
> +## LOCALIZATION NOTE (rooms_list_current_conversations): We prefer to have no
> +## number in the string, but if you need it for your language please use {{num}}.
> +## XXX this is currently broken, see comments in bug 1074672.

Can you say _what_ is currently broken?

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

Fixed the issue with l10n by creating two distinct strings. Ready for final review.

Comment 17

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Woops, missed your comment notification, will address remaining ones asap.

Comment 18

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop.

Addressed review comments.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8501088 [details] [diff] [review]
Implement a room list view for Loop.

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

::: browser/components/loop/content/js/panel.jsx
@@ +23,5 @@
>  
>    var TabView = React.createClass({
> +    propTypes: {
> +      buttonsHidden: React.PropTypes.bool,
> +      selectedTab: React.PropTypes.string // Used by the UI showcase

nit: please put this comment atop the line it refers to

@@ +478,5 @@
> +
> +    render: function() {
> +      var room = this.props.room;
> +      var pillClasses = React.addons.classSet({
> +        "room-pill": true,

instead of 'pill' I'd prefer we'd go with the more commonly used 'notification'. Additionally I don't see these styles defined in panel.css yet. Is there a rooms.css you didn't `hg add`?

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +37,3 @@
>        setLoopCharPref: sandbox.stub(),
>        getLoopCharPref: sandbox.stub().returns("unseen"),
> +      getPluralForm: function() {

I don't think you need this anymore...

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8501088 [details] [diff] [review]
Implement a room list view for Loop.

r=me with comments addressed.

Thanks NiKo`!

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> instead of 'pill' I'd prefer we'd go with the more commonly used
> 'notification'

Fixed, and added styles for it.

> > +      getPluralForm: function() {
> I don't think you need this anymore...

We do, because getPluralForm is still used for the "rooms_list_current_conversations" string.

Also and after discussing about store with :dmose yesterday evening, in this new version of the patch I add support for errors right in RoomListStore; now any error sent by the mozLoop API for rooms will be reflected in the store state, so it'll be easier to display some error message in a component. Also refctored the tests a little so we can easily insert test fixtures in there.

Re-requesting a review for these changes.

Comment 22

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Note: Part 2 will address these remaining bits:

* To obtain the data from the rooms impl in the backend via mozLoop API (depends on bug 1074664)
* Should update the list when notified by the rooms impl (depends on bug 1074664)

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8501677 [details] [diff] [review]
Implement a room list view for Loop - Part 1

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

Knowing mmaslaney and darrin's work a bit, I'm pretty sure they want offsets to be in pixels instead of (r)ems.

I'm quite fine with that being dealt with in a follow-up, but I prefer to have that be part of this patch to avoid magic floating point rem values.

::: browser/components/loop/content/shared/css/panel.css
@@ +151,5 @@
> +  background: transparent;
> +  width: 8px;
> +  height: 8px;
> +  border-radius: 50%;
> +  margin-right: .3rem;

these kind of fractional values smell like they should really be fixed pixel values. Being scalable is nice, but we don't really want everything to be dependent on font-size, right?

@@ +174,5 @@
> +
> +.room-list > .room-entry > p > a {
> +  color: #777;
> +  opacity: .5;
> +  transition: opacity 0.1s ease-in-out 0s;

I think you can omit the 0 in 0.1s here and omit the 's' in 0s

Comment 24

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Implement a room list view for Loop - Part 1

Addressed comments, rebased on top of latest fx-team.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mike de Boer [:mikedeboer] from comment #23)
> 
> Knowing mmaslaney and darrin's work a bit, I'm pretty sure they want offsets
> to be in pixels instead of (r)ems.
>
> I'm quite fine with that being dealt with in a follow-up, but I prefer to
> have that be part of this patch to avoid magic floating point rem values.

I've had multiple explicit discussions with Darrin about this, and he was quite clear that in the case of at least the first set of mockups, his stuff was intended to convey how things were supposed to look, and that he was very much ok with us changing units in the interest of maintainability.  If things are different for the most recent set of mockups, that'd be great to know.  Sounds like the follow-up patch wants ui-review and/or some f2f discussion.

> ::: browser/components/loop/content/shared/css/panel.css
> @@ +151,5 @@
> > +  background: transparent;
> > +  width: 8px;
> > +  height: 8px;
> > +  border-radius: 50%;
> > +  margin-right: .3rem;
> 
> these kind of fractional values smell like they should really be fixed pixel
> values. Being scalable is nice, but we don't really want everything to be
> dependent on font-size, right?

Scalability has high value, and a fairly effective heuristic to help achieve it is to stick to relative units except in cases where things very clearly _shouldn't_ be scaled relative to font-size.  In other words, fractional rems are often a pretty good idea.  

If your concern is that the value is "magic", in the sense that it's an esthetic choice whose derivation can't easily be reasoned about, that's true, but it's just as true of absolute pixel values, as is made clear by our need to go back to talk to the designers.  This would seem best addressed with a comment.

Is there some other explicit concern you have mind that I'm missing?
 
> @@ +174,5 @@
> > +
> > +.room-list > .room-entry > p > a {
> > +  color: #777;
> > +  opacity: .5;
> > +  transition: opacity 0.1s ease-in-out 0s;
> 
> I think you can omit the 0 in 0.1s here and omit the 's' in 0s

Unfortunately, omitting in s in '0s' is causes the parseable CSS mochitest to origin, so I'm going to revert that change before I land this patch.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Part 1 - Implement a room list view for Loop

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8501980 [details] [diff] [review]
Part 1 - Implement a room list view for Loop

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

Reverted 0s CSS change as documented above, carrying forward mike's review.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Since there will be a part 2, adding leave-open so the sheriffs don't close this after uplifting part 1.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

For unclear reasons, this test made an intermitment failure in our call functional test (bug 1080095) more frequent.  I extended the length of our workaround timeout from 2s to 5s, which is seems sufficient to generally avoid the problem on my machine.  One more patch iteration, coming up.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [checked in] Part 1 - Implement a room list view for Loop

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 33

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 2 - Loop Rooms tab to reuse the Call one.

As per comment #1 on bug 1077792 [0], this patch reuses the bubble icon from the Call tab for the Rooms one.

That means duplicate tab icons, but as the Rooms feature is disabled (and hidden) by default, this is not much of an issue. 

Still to be addressed in an upcoming Part 3:

* To obtain the data from the rooms impl in the backend via mozLoop API
* Should update the list when notified by the rooms impl

[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1077792#c1

Comment 34

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

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

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8501998 [details] [diff] [review]
[checked in] Part 1 - Implement a room list view for Loop

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 36

[Francesco Lodolo [:flod]]

Localization comment for this string is incomplete (but thanks for the comment about {num}!)
http://hg.mozilla.org/mozilla-central/diff/c7a7563e5afe/browser/locales/en-US/chrome/browser/loop/loop.properties

See for reference
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Use_proper_plural_forms

As it is, tools won't be able to recognize it as a plural form

Comment 37

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: [landed on fx-team] Part 2 - Loop Rooms tab to reuse the Call one.

Addressed comment from :flod (Comment #36)

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 8503109 [details] [diff] [review]
[landed on fx-team] Part 2 - Loop Rooms tab to reuse the Call one.

https://hg.mozilla.org/integration/fx-team/rev/984a9d8ac938

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/984a9d8ac938

Comment 40

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 3 - Loop room list to use the mozLoop.rooms API.

This patch makes the roomListStore using the mozLoop.rooms API landed recently. Also fixed borken UI showcase.

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 8511923 [details] [diff] [review]
Part 3 - Loop room list to use the mozLoop.rooms API.

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

This is almost good enough for r+, but I'd just like to check it again after the currSize -> participants changes.

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +24,5 @@
>      roomToken: String,
>      roomUrl:   String,
>      roomName:  String,
>      maxSize:   Number,
>      currSize:  Number,

This will need to change for the currSize -> participants parameter change.

::: browser/components/loop/ui/fake-mozLoop.js
@@ +6,5 @@
> +  roomToken: "_nxD4V4FflQ",
> +  roomUrl: "http://sample/_nxD4V4FflQ",
> +  roomName: "First Room Name",
> +  maxSize: 2,
> +  currSize: 0,

This will need to change for the currSize -> participants parameter change (and the ones below)

::: browser/components/loop/ui/index.html
@@ +10,5 @@
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/common.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/conversation.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/panel.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/contacts.css">
> +    <link rel="stylesheet" type="text/css" href="../standalone/content/css/webapp.css">

As per irc, I suggest we move these to a different bug, as the case of loading via your own server is broken and needs a more involved fix.

Comment 42

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 3 - Loop room list to use the mozLoop.rooms API.

Addressed comments. Filed bug 1089722 to addressed proper validation of deleted rooms, as discussed on IRC.

Comment 43

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 3 - Loop room list to use the mozLoop.rooms API.

Forgot to update a test.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 8512069 [details] [diff] [review]
Part 3 - Loop room list to use the mozLoop.rooms API.

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

Just one bit you forgot to undo. Otherwise looks good, r=Standard8.

::: browser/components/loop/ui/index.html
@@ +10,5 @@
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/common.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/conversation.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/panel.css">
>      <link rel="stylesheet" type="text/css" href="../content/shared/css/contacts.css">
> +    <link rel="stylesheet" type="text/css" href="../standalone/content/css/webapp.css">

Looks like you forgot to undo this line.

Comment 45

[Mark Banner (:standard8)]

I landed the patch with the additional line removed:

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

Comment 46

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

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

Comment 47

[u279076]

[Tracking Requested - why for this release]: nominated to track since this is a Loop 35 blocker.

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c7faac329686

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8512069 [details] [diff] [review]
Part 3 - Loop room list to use the mozLoop.rooms API.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

This patch introduced usages of .5rem for padding-left and padding-right, when we were previously using 14px for the left and right (http://hg.mozilla.org/mozilla-central/diff/c7a7563e5afe/browser/components/loop/content/shared/css/panel.css#l1.20 for one example).

Why was this change made without updating the other places that were using px values? It should have been r- due to that.

(In reply to Dan Mosedale (:dmose) - away from bugzilla until Dec 1 from comment #25)
> > > +  transition: opacity 0.1s ease-in-out 0s;
> > 
> > I think you can omit the 0 in 0.1s here and omit the 's' in 0s
> 
> Unfortunately, omitting in s in '0s' is causes the parseable CSS mochitest
> to origin, so I'm going to revert that change before I land this patch.

Time durations and angles must have the unit of value appended and cannot have the unit dropped when the value is 0. The current behavior of dropping the unit for values of zero on lengths is for historical reasons and backwards-compat. This was discussed during the standards process for transitions and animations, and was decided to require the unit for values of <zero> when used for time durations. See https://bugzilla.mozilla.org/show_bug.cgi?id=627559 comment 2 through 4 for more context.

Comment 51

[Paul Silaghi, QA [:pauly]]

Verified fixed 36.0a2 (2014-12-17), 35b4 Win 7