Implement a general Room list view in Loop

VERIFIED FIXED in Firefox 35

Status

enhancement
P1
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: standard8, Unassigned)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox35 verified, firefox36 verified)

Details

(Whiteboard: [rooms][strings], )

User Story

* As a user, I should have access to the list of rooms I have created, so I have easy access to them.
** Note: this should not remove the existing "share this link" UI, and should be hidden/hidden by pref until the rooms feature is fully implemented

UX:
* If there are no rooms in the list, display "No Current Conversations"
* Display list of room names and links as per https://people.mozilla.org/~dhenein/loop/rooms/room-hover.html (replacing "Active Rooms" with "Current Conversations" (copy/delete/rename options to come later)
* "Current Conversations" are listed in reverse order of latest expiry time

Strings:
* (Plural Form, L10n note: prefer to have no number in the string, but if you need it for your language please use <appropriate text that gets replaced>), No Current Conversations / Current Conversations

Implementation:
* Need appropriate Rooms List Views
* To obtain the data from the rooms impl in the backend via mozLoop API
* Should update the list when notified by the rooms impl

Attachments

(4 attachments, 16 obsolete attachments)

194.56 KB, image/png
Details
71.04 KB, patch
dmose
: review+
Details | Diff | Splinter Review
2.62 KB, patch
standard8
: review+
Details | Diff | Splinter Review
15.83 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Posted image Expected UX
No description provided.
Reporter

Updated

5 years ago
User Story: (updated)
Whiteboard: [rooms] → [rooms][strings]
Reporter

Updated

5 years ago
Depends on: 1074665
Reporter

Updated

5 years ago
Severity: normal → enhancement
User Story: (updated)
Reporter

Comment 1

5 years ago
(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.
Flags: needinfo?(rtestard)
(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.
User Story: (updated)
Flags: needinfo?(rtestard)

Updated

5 years ago
User Story: (updated)
(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.
This is for getting early feedback about the initial implementation of a room store. No tests yet, consider this as a sandbox.
Attachment #8499546 - Flags: feedback?(standard8)
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.
Attachment #8499546 - Attachment is obsolete: true
Attachment #8499546 - Flags: feedback?(standard8)
Attachment #8499556 - Flags: feedback?(standard8)
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.
Attachment #8499556 - Flags: feedback?(standard8)
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.
Attachment #8499556 - Attachment is obsolete: true
Attachment #8500409 - Flags: review?(standard8)
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.
Attachment #8500409 - Attachment is obsolete: true
Attachment #8500409 - Flags: review?(standard8)
Attachment #8500411 - Flags: review?(standard8)
Reporter

Updated

5 years ago
Depends on: 1078311
Following discussion on IRC, there's now a dedicated store for room list (roomStore.js has been renamed to roomListStore.js).
Attachment #8500411 - Attachment is obsolete: true
Attachment #8500411 - Flags: review?(standard8)
Attachment #8500526 - Flags: review?(standard8)
Reporter

Comment 10

5 years ago
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}}
Attachment #8500411 - Attachment is obsolete: false
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.
Attachment #8500411 - Attachment is obsolete: true
Attachment #8500526 - Attachment is obsolete: true
Attachment #8500526 - Flags: review?(standard8)
Attachment #8500616 - Flags: review?(standard8)
Some fixes. Previous comment about l10n remains.
Attachment #8500616 - Attachment is obsolete: true
Attachment #8500616 - Flags: review?(standard8)
Attachment #8500646 - Flags: review?(standard8)
Reporter

Comment 13

5 years ago
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.
Attachment #8500646 - Flags: review?(standard8)
Attachment #8500646 - Flags: review?(mdeboer)
Attachment #8500646 - Flags: review?(dmose)
Patch rebased on top of latest fx-team.
Attachment #8500646 - Attachment is obsolete: true
Attachment #8500646 - Flags: review?(mdeboer)
Attachment #8500646 - Flags: review?(dmose)
Attachment #8500943 - Flags: review?(mdeboer)
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?
Attachment #8500943 - Flags: review?(mdeboer) → feedback+
Fixed the issue with l10n by creating two distinct strings. Ready for final review.
Attachment #8500943 - Attachment is obsolete: true
Attachment #8501067 - Flags: review?(mdeboer)
Woops, missed your comment notification, will address remaining ones asap.
Attachment #8501067 - Flags: review?(mdeboer)
Addressed review comments.
Attachment #8501067 - Attachment is obsolete: true
Attachment #8501088 - Flags: review?(mdeboer)
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...
Attachment #8501088 - Flags: review?(mdeboer)
Comment on attachment 8501088 [details] [diff] [review]
Implement a room list view for Loop.

r=me with comments addressed.

Thanks NiKo`!
Attachment #8501088 - Flags: review+
(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.
Attachment #8501088 - Attachment is obsolete: true
Attachment #8501677 - Flags: review?(mdeboer)
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)
Depends on: 1074664
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
Attachment #8501677 - Flags: review?(mdeboer) → review+
Addressed comments, rebased on top of latest fx-team.
Attachment #8501677 - Attachment is obsolete: true
Attachment #8501863 - Flags: review+
(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 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.
Attachment #8501980 - Flags: review+
Since there will be a part 2, adding leave-open so the sheriffs don't close this after uplifting part 1.
Keywords: leave-open
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.
Attachment #8501998 - Attachment description: Part 1 - Implement a room list view for Loop → (landed on fx-team) Part 1 - Implement a room list view for Loop
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
Attachment #8502375 - Flags: review?(standard8)
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
Attachment #8501998 - Flags: approval-mozilla-aurora?
Attachment #8501998 - Flags: approval-mozilla-aurora?
Reporter

Updated

5 years ago
Attachment #8502375 - Flags: review?(standard8) → review+
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
Reporter

Updated

5 years ago
Attachment #8503109 - Flags: review?(standard8) → review+
Reporter

Updated

5 years ago
Attachment #8501998 - Attachment description: (landed on fx-team) Part 1 - Implement a room list view for Loop → [checked in] Part 1 - Implement a room list view for Loop
Reporter

Updated

5 years ago
Assignee: nobody → nperriault
Reporter

Comment 38

5 years ago
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
Attachment #8503109 - Attachment description: Part 2 - Loop Rooms tab to reuse the Call one. → [landed on fx-team] Part 2 - Loop Rooms tab to reuse the Call one.

Updated

5 years ago
backlog: --- → Fx35+
This patch makes the roomListStore using the mozLoop.rooms API landed recently. Also fixed borken UI showcase.
Attachment #8511923 - Flags: review?(standard8)
Reporter

Comment 41

5 years ago
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.
Attachment #8511923 - Flags: review?(standard8) → review-
Addressed comments. Filed bug 1089722 to addressed proper validation of deleted rooms, as discussed on IRC.
Attachment #8511923 - Attachment is obsolete: true
Attachment #8512057 - Flags: review?(standard8)
Forgot to update a test.
Attachment #8512057 - Attachment is obsolete: true
Attachment #8512057 - Flags: review?(standard8)
Attachment #8512069 - Flags: review?(standard8)
Reporter

Comment 44

5 years ago
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.
Attachment #8512069 - Flags: review?(standard8) → review+
Reporter

Comment 45

5 years ago
I landed the patch with the additional line removed:

https://hg.mozilla.org/integration/fx-team/rev/be2cb4fa54e3
Keywords: leave-open
Target Milestone: --- → mozilla36

Updated

5 years ago
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/be2cb4fa54e3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
No longer depends on: 1074665

Updated

5 years ago
Iteration: --- → 36.1
Reporter

Updated

5 years ago
Blocks: 1074679
[Tracking Requested - why for this release]: nominated to track since this is a Loop 35 blocker.
Flags: qe-verify+
Flags: in-testsuite?
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
Attachment #8512069 - Flags: approval-mozilla-aurora?
Attachment #8512069 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Verified fixed 36.0a2 (2014-12-17), 35b4 Win 7
Status: RESOLVED → VERIFIED
Depends on: 1113064
Depends on: 1118393
Reporter

Updated

4 years ago
Flags: in-testsuite? → in-testsuite+
Reporter

Updated

4 years ago
You need to log in before you can comment on or make changes to this bug.