Closed Bug 902981 Opened 7 years ago Closed 7 years ago

[Music] [User Story] Provide access to music controls in the notifications tray

Categories

(Firefox OS Graveyard :: Gaia::Music, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
1.3 Sprint 3 - 10/25

People

(Reporter: skasetti, Assigned: squib)

References

Details

(Keywords: feature, Whiteboard: [1.3:p2, ucid: media40])

Attachments

(4 files)

User Story:

As a user, I want to access the controls to the music app in a notification tray or enter the music app by swiping on the play icon on the status bar while the homescreen is still unlocked
Depends on: 902974
Taking and setting blocking bug (this should happen before lockscreen controls).
Blocks: 891024
Status: NEW → ASSIGNED
Depends on: 904125
Assignee: nobody → squibblyflabbetydoo
This is essentially done, and I'm just waiting for bug 902974 to land now. For anyone who'd like to follow along, the code is here: https://github.com/jimporter/gaia/compare/music-nowplaying-notification...music-notification-controls
Ok, this is done. Note that the PR has the commits from bug 902981 in it.
Attachment #807192 - Flags: ui-review?(rmacdonald)
Attachment #807192 - Flags: review?(dflanagan)
Attachment #807192 - Flags: review?(alive)
(In reply to Jim Porter (:squib) from comment #3)
> Created attachment 807192 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12314
> 
> Ok, this is done. Note that the PR has the commits from bug 902981 in it.

Wait, this is bug 902981. And the PR just has one commit in it, so I'm confused.  Also, do you want Alive and I to both revhew the whole patch, or are you asking me to review the music changes and Alive to review the system changes?
Flags: needinfo?(squibblyflabbetydoo)
(In reply to David Flanagan [:djf] from comment #4)
> (In reply to Jim Porter (:squib) from comment #3)
> > Created attachment 807192 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/12314
> > 
> > Ok, this is done. Note that the PR has the commits from bug 902981 in it.
> 
> Wait, this is bug 902981. And the PR just has one commit in it, so I'm
> confused.

Sorry, I meant bug 902974. The PR has only one commit in it now because bug 902974 landed, so you won't see its commits in the PR anymore.

> Also, do you want Alive and I to both revhew the whole patch, or
> are you asking me to review the music changes and Alive to review the system
> changes?

I think it makes the most sense for you to review the music changes (namely, shared/js/media/remote_controls.js), and Alive to review the system changes. I think it might make sense for both of you to look over the tests though, since the fake music app is supposed to be similar to the real music app.
Flags: needinfo?(squibblyflabbetydoo)
Oh, as a note to reviewers, I'll be updating this with a few small changes in the system app, namely to allow fast-forwarding by holding the next or previous buttons (like the player in the music app and the bluetooth controls).
Hm, maybe not. For some reason, I can't manage to detect a long-press on the controls (I'm listening for contextmenu), even though it works on other elements. Does anyone have any ideas why that would be? The only difference I can see is that it works on a div, but not on a button.
Duplicate of this bug: 887624
Comment on attachment 807192 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12314

Sorry this took so long.  r+.

This was my first time reviewing IAC code, so it was pretty interesting for me.
Attachment #807192 - Flags: review?(dflanagan) → review+
Attachment mime type: text/plain → text/x-github-pull-request
Duplicate of this bug: 805946
Attached image What it looks like
Here's what the patch looks like in action. Hopefully this is enough detail; I could film a video of this in action, but that'd be more work. :)
Attachment #819910 - Flags: ui-review?(rmacdonald)
Attachment #807192 - Flags: ui-review?(rmacdonald)
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Keywords: feature
I will take a look at this today or tomorrow but, in the meantime, I'll flag Eric for visual design review.
Flags: needinfo?(epang)
Hey Jim, this is looking good :).  I just have a few suggestions. I've attached an image with a few small tweaks to update the layout and highlight states.  Let me know what you think or if you have any questions. Thanks!
Flags: needinfo?(epang) → needinfo?(squibblyflabbetydoo)
(In reply to Eric Pang [:epang] from comment #14)
> Hey Jim, this is looking good :).  I just have a few suggestions. I've
> attached an image with a few small tweaks to update the layout and highlight
> states.  Let me know what you think or if you have any questions. Thanks!

Ok, I've updated the CSS to match this. Good catch on the border-radius for the buttons! I didn't see it on a desktop build; must be some sort of build system weirdness.

The smaller album cover looks really strange to me, though. Maybe that's just because I've been staring at the old version so long, so I'll defer to you here. :)
Flags: needinfo?(squibblyflabbetydoo)
Two more small questions that I just thought of:

1) Users can tap the "now playing" info at the top to go to the music app. Should there be an on-tap state for that like how the buttons turn blue?

2) The (wrong) blue color I used for the buttons came from the music app. Should I change it to #00aacc there as well?
Flags: needinfo?(epang)
Comment on attachment 819910 [details]
What it looks like

Looks great. Thanks for all of your hard work on this!
Attachment #819910 - Flags: ui-review?(rmacdonald) → ui-review+
Landed: https://github.com/mozilla-b2g/gaia/commit/36048198ae3f87671ced8c0179bea1e029ec7d9c

Thanks everyone for reviews!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Jim Porter (:squib) from comment #16)
> Two more small questions that I just thought of:
> 
> 1) Users can tap the "now playing" info at the top to go to the music app.
> Should there be an on-tap state for that like how the buttons turn blue?
> 
> 2) The (wrong) blue color I used for the buttons came from the music app.
> Should I change it to #00aacc there as well?

Hey Jim,

Sorry for the delayed response on this!  Hope it isn't too late..

1. The notification should have a hit state in general for row items.  I previously opened this bug about it: 917860

2.  Yes if you can update the highlight state to #00aacc that would be great

Thanks again for all the work you've done on this!
Flags: needinfo?(epang)
Flags: in-moztrap?
(In reply to Eric Pang [:epang] from comment #19)
> 1. The notification should have a hit state in general for row items.  I
> previously opened this bug about it: 917860

I hate to be a bother, but could you update attachment 821630 [details] to cover this? I want to make sure I get it right. Specifically, the things I'm most concerned about are:

1) How much of the now playing widget should get highlighted? I think we should highlight the area that you can tap to open the music app, but this means that, with the margins the way they are, the music icon isn't vertically centered in the highlighted area. This looks a bit strange.

2) Should the highlight state on the controls match the highlight state for the notifications? That would be more consistent, I think...

I'll probably end up doing most of these changes in bug 891024, which is a followup to this bug anyway.

> 2.  Yes if you can update the highlight state to #00aacc that would be great

I'll file a bug on this later today.
Hey Jim, sorry for getting back to you about this so late, I didn't see the msg, next time ni me :).

1) How much of the now playing widget should get highlighted? I think we should highlight the area that you can tap to open the music app, but this means that, with the margins the way they are, the music icon isn't vertically centered in the highlighted area. This looks a bit strange.

- I was thinking that the entire row gets highlighted since that's how the other notifications function.  We can still use only the music icon area as the hit area, but when pressed highlight the entire notification. Currently the highlight is #b1f1ff at 60% opacity.

2) Should the highlight state on the controls match the highlight state for the notifications? That would be more consistent, I think...

- I agree this is a good idea for consistency :)
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Eric Pang [:epang] from comment #21)
> Hey Jim, sorry for getting back to you about this so late, I didn't see the
> msg, next time ni me :).

Noted.

> - I agree this is a good idea for consistency :)

I filed bug 938502 to track this. I'll post screenshots for you to look at once I make the changes. Thanks!
Flags: needinfo?(squibblyflabbetydoo)
Whiteboard: [1.3:p2, ucid: media40]
Verified fixed on Buri using:

Gaia   2cc78d696482e0434b584f5645af55e3105e59a2
SourceStamp 3c4fc4279e6a
BuildID 20131122123502
Version 28.0a1
Base image: 11-15

Have added a few test cases to Moztrap, but when I finish I will change that flag.
Status: RESOLVED → VERIFIED
#10617 added to Moztrap.
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.