Closed
Bug 1015247
Opened 10 years ago
Closed 10 years ago
[Music] Update to use gaia-header
Categories
(Firefox OS Graveyard :: Gaia::Music, defect)
Tracking
(ux-b2g:2.1)
RESOLVED
FIXED
ux-b2g | 2.1 |
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(4 files)
No description provided.
Blocks: gaia-header
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8431691 -
Flags: review?(yor)
Assignee | ||
Updated•10 years ago
|
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo)
Comment on attachment 8431691 [details] pull-request (master) Code looks good. On use "!important" for the custom icon, check out the changes to gaia_header/style.css I'm doing here: https://github.com/mozilla-b2g/gaia/pull/19743/files That should make it possible to set custom icon image.
Attachment #8431691 -
Flags: review?(yor) → review+
Assignee | ||
Comment 3•10 years ago
|
||
squib: Comments addressed :)
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo) → feedback?(squibblyflabbetydoo)
Comment 4•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
Unfortunately, this doesn't seem to work right. See my comments on GitHub.
Attachment #8431691 -
Flags: feedback?(squibblyflabbetydoo) → feedback-
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 5•10 years ago
|
||
squib: Due to some of the difficulties around web-component platform support we went ahead and made the header changes to the existing header building-block. This meant that is conflicted with our web-component patches (like this one). In v2.1 we will be merging the building-block work into the gaia-header web-component, then this patch will need amending. Sorry for the confusion!
Assignee | ||
Updated•10 years ago
|
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Comment 6•10 years ago
|
||
squib: Patch looks good to me in normal use, but need some info from you on how to invoke the Music app via an activity to check all looks good there. As gaia-header ships with the gaia-icons icon-font, I thought it would be worth switching out the other PNGs while I was at it.
Flags: needinfo?(squibblyflabbetydoo)
Comment 7•10 years ago
|
||
If you want to load the music app as a pick activity, the easiest way is probably Settings -> Sounds -> Manage Ringtones -> "+", which will let you pick a song to turn into a ringtone.
Flags: needinfo?(squibblyflabbetydoo)
Comment 8•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
Some issues I see with the latest version:
* The "play" icon at the top right shows up even when there's nothing playing. It should be hidden in this case. This also applies to the pick activity.
* When the app starts up, the first tab at the bottom (the mix tab) isn't colored blue. Selecting others tabs colors the icon blue, though.
* Tapping the "play" icon at the top right to return to the "now playing" card results in a weird glitch in the bottom left corner. Part of the "mix" icon is visible.
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo) → review-
Assignee | ||
Updated•10 years ago
|
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo)
Attachment #8431691 -
Flags: review?(dkuo)
Attachment #8431691 -
Flags: review-
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Jim Porter (:squib) from comment #8) > Comment on attachment 8431691 [details] > pull-request (master) > > Some issues I see with the latest version: > > * The "play" icon at the top right shows up even when there's nothing > playing. It should be hidden in this case. This also applies to the pick > activity. Fixed this to be hidden by default. > * When the app starts up, the first tab at the bottom (the mix tab) isn't > colored blue. Selecting others tabs colors the icon blue, though. Fixed this by making sure the app starts at index.html#mix > * Tapping the "play" icon at the top right to return to the "now playing" > card results in a weird glitch in the bottom left corner. Part of the "mix" > icon is visible. I can't see this glitch. I see the player slide in from the right to cover the currently active UI.
Comment 10•10 years ago
|
||
Wilson, I found this is actually a big patch, including the bower part which I need to figure out how it works, so it might take some time for me to review it, just let you know first :)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Dominic Kuo [:dkuo] from comment #10) > Wilson, I found this is actually a big patch, including the bower part which > I need to figure out how it works, so it might take some time for me to > review it, just let you know first :) FWIW you can ignore everything inside bower_components/. As I said if you don't want to use Bower I can refactor the patch to use shared/.
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
Updates:
- Fixed 'open' activity
- Updated to latests icon-font ready for performance boost when gecko patch lands
- Rebased against master
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Comment on attachment 8431691 [details] pull-request (master) Wilson, Thanks for addressing the previous issues, though I sill found some and please see attachment 8470665 [details], here are the issues after the updated patch: 1. I think the back button has cover a little bit on the song title, it's probably the italic issue and maybe we should give a margin to prevent that. 2. If the song title overflows, it should be ellipsis. 3. The player icon should be hidden when it's in the player page. 4. The background color seems different from the current one. These are noticeable issues so we better address them before landing it, would you please check it again? thanks!
Attachment #8431691 -
Flags: review?(dkuo)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Dominic Kuo [:dkuo] from comment #14) > 1. I think the back button has cover a little bit on the song title, it's > probably the italic issue and maybe we should give a margin to prevent that. > 2. If the song title overflows, it should be ellipsis. The new header 'font-fit' logic takes care of the text sizing. It's quite complex, but essentially it will shrink text until it can't shrink anymore, then it will apply ellipsis. I think positioning in attachment 8470665 [details] is correct, but there is some strange sub-pixel font rendering issue on the first and last letters. This looks like a platform/rendering issue. I'm not really sure how to address this. > 3. The player icon should be hidden when it's in the player page. Ah, I was unaware of this, I will fix. > 4. The background color seems different from the current one. This is by design. All media apps should have the same header color now. This instruction is from Visual Team.
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
Updated
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
dkuo: all looks good to me now, could you take another look?
Attachment #8431691 -
Flags: review?(squibblyflabbetydoo) → review?(dkuo)
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
Wilson, thanks for addressing those issues in comment 14, though I still see the title is not ellipsis in attachment 8472840 [details], looks like the 'font-fit' logic miscalculated the width then caused this problem?
Flags: needinfo?(dkuo) → needinfo?(wilsonpage)
Assignee | ||
Comment 21•10 years ago
|
||
I got to the bottom of the issue. If was related to `text-overflow: hidden` not working on `display: flex` elements.
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
UPDATED
- Fixed font-fit logic in gaia-header and updated to v0.3.6
- Updated gaia-icons v0.5.0
Assignee | ||
Comment 23•10 years ago
|
||
dkuo: Think were good now. See attachment 8472960 [details].
Flags: needinfo?(dkuo)
Assignee | ||
Comment 24•10 years ago
|
||
I also noticed the search button had a strange default background shaded background image on so this has been fixed too.
Comment 25•10 years ago
|
||
Wlison, I think we almost there and I would like to take a last look before r+ing this patch, would you please address those new issues I found on github then notify me again? thanks!
Flags: needinfo?(dkuo)
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
UPDATED
- Changed manifest to point entry-points to index.html#mix
- Fixed missing shuffle and play icons in artist song list
- Removed commented out CSS
- Removed extra `navigator.mozl10n.get()` call
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(dkuo)
Comment 27•10 years ago
|
||
Marking this bug for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan. Just updating the flags appropriately for release tracking.
ux-b2g: --- → 2.1
Comment 28•10 years ago
|
||
Comment on attachment 8431691 [details] pull-request (master) Thanks a lot, Wilson! I think the patch looks good to me now, only some minor issues to address before landing it. Also bug 837675 is landed, do you think it's possible for us to measure the startup time before/after this patch applied? I can do it if you are still working on the other gaia-header patches.
Attachment #8431691 -
Flags: review?(dkuo) → review+
Flags: needinfo?(dkuo)
Updated•10 years ago
|
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8431691 [details]
pull-request (master)
UPDATED
- Addressed all Github comments
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 30•10 years ago
|
||
dkuo: It would be great if you could look at startup perf! You must make sure that Music app is using the same version of gaia-icons as shared/elements/gaia-icons to get maximum startup perf. This is because Gecko pre-loads the shared/ gaia-icons font. You can check the gaia-icons/bower.json to be sure. You make need to do a `make reset-gaia` to make sure the pre-loading has taken place. Ping me if you have any questions.
Comment 31•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #30) > dkuo: It would be great if you could look at startup perf! You must make > sure that Music app is using the same version of gaia-icons as > shared/elements/gaia-icons to get maximum startup perf. This is because > Gecko pre-loads the shared/ gaia-icons font. You can check the > gaia-icons/bower.json to be sure. > > You make need to do a `make reset-gaia` to make sure the pre-loading has > taken place. Ping me if you have any questions. Thanks Wilson, I think I should be able to perf the startup time tomorrow and will update the status here.
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8431691 [details] pull-request (master) LANDED https://github.com/mozilla-b2g/gaia/commit/313384274a7158d222f41e6fe9a2ebb149bd8049
Assignee | ||
Updated•10 years ago
|
Attachment #8431691 -
Flags: feedback-
Assignee | ||
Comment 33•10 years ago
|
||
I've had to land as we committed to landing by today. We'll use Datazilla to track any performance regressions and handle them in the stability sprint.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•