1015296 - [Ringtones] Update to use gaia-header

Status: RESOLVED FIXED

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

Comment 1

[Jim Porter (:squib)]

We should probably wait until bug 960329 is fixed before working on this.

Comment 2

[Yan Or]

Attachment: Pull request

Comment 3

[Wilson Page [:wilsonpage]]

Comment on attachment 8431363 [details] [review]
Pull request

Looks good, Yan's da Man!

- Need to rebase and remove `data-` prefixes
- Noticed that we need to remove `data-` from <gaia-subheader>, should that be a separate issue, or bundled with this?
- See Github comments

Comment 4

[Jim Porter (:squib)]

Comment on attachment 8431363 [details] [review]
Pull request

This code looks sane overall, but it's currently causing test failures, so we'll need to get those fixed before this can land.

Comment 5

[Wilson Page [:wilsonpage]]

Comment on attachment 8431363 [details] [review]
Pull request

This patch need reworking following changes in v2.0 and updated import workflow.

Comment 6

[Wilson Page [:wilsonpage]]

Yan, what's the status on this?

Comment 7

[Yan Or]

Comment on attachment 8431363 [details] [review]
Pull request

Updated based on latest master and all tests passed.

Comment 8

[Stephany Wilkes]

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.

Comment 9

[Stephany Wilkes]

This review has not moved for a couple of months. We are in sprint 3 of 2.1 and this item blocks 2.1. We need reviews to move so that these items can land *this week*, before Friday 8/22.

Squib, please reassign or review ASAP (+ Hema to advise on reassignment if needed).

Let me know if anything else is blocking here. Thanks!

Comment 10

[Yan Or]

Actually Jim did review this the first time around back in May.  I just resubmitted for final review last week.  But yes, we need to get the review done soon if we hope to land by Friday.

Comment 11

[Jim Porter (:squib)]

Comment on attachment 8431363 [details] [review]
Pull request

I tried the patch out and noticed a few significant problems:

1) The "+" icon doesn't show up in the "Manage Ringtones" activity.
2) The colors for the header have changed; everything seems darker now. Most importantly, this includes the "Done" label in the picker, which doesn't initially show up as disabled.
3) There appears to be a *huge* performance regression in scrolling the lists, as well as some visual glitches while scrolling (parts of the background show up as black).

Without these issues resolved, there's no way we can land this for 2.1.

Comment 12

[Hema Koka [:hema]]

Stephany, Yan and Jim have already answered, clearing my NI

Comment 13

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 14

[Wilson Page [:wilsonpage]]

(In reply to Jim Porter (:squib) from comment #11)
> Comment on attachment 8431363 [details] [review]
> Pull request
> 
> I tried the patch out and noticed a few significant problems:
> 
> 1) The "+" icon doesn't show up in the "Manage Ringtones" activity.

Fixed

> 2) The colors for the header have changed; everything seems darker now.

Colors are not tied to this patch, they come from the central gaia-theme CSS. I'm working with visual to get the exact color changes they require for v2.1. Once this is resolved all the apps will update, so this should block this patch landing.

> Most importantly, this includes the "Done" label in the picker, which doesn't
> initially show up as disabled.

Fixed

> 3) There appears to be a *huge* performance regression in scrolling the
> lists, as well as some visual glitches while scrolling (parts of the
> background show up as black).

Fixed

Comment 15

[Jim Porter (:squib)]

Comment on attachment 8476075 [details] [review]
pull-request (master)

Ok, things look good. It seems like scrolling perf is still worse than it was pre-patch, but it's not a big issue anymore.

Comment 16

[Jim Porter (:squib)]

Oh, and I had one nit on GitHub.

Comment 17

[Wilson Page [:wilsonpage]]

Comment on attachment 8476075 [details] [review]
pull-request (master)

LANDED https://github.com/mozilla-b2g/gaia/commit/13785d9181804b095ba49444e31237d442675228