Yan Or Reporter
	Description • 10 years ago

      No description provided.

	Jim Porter (:squib)
	Comment 1 • 10 years ago

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

	Wilson Page [:wilsonpage] Assignee
	Comment 3 • 10 years ago

Comment on attachment 8431363 [details]
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

	Jim Porter (:squib)
	Comment 4 • 10 years ago

Comment on attachment 8431363 [details]
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.

	Wilson Page [:wilsonpage] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8431363 [details]
Pull request

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

	Wilson Page [:wilsonpage] Assignee
	Comment 6 • 10 years ago

Yan, what's the status on this?

	Yan Or Reporter
	Comment 7 • 10 years ago

Comment on attachment 8431363 [details]
Pull request

Updated based on latest master and all tests passed.

	Stephany Wilkes
	Comment 8 • 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.

	Stephany Wilkes
	Comment 9 • 10 years ago

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!

	Yan Or Reporter
	Comment 10 • 10 years ago

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.

	Jim Porter (:squib)
	Comment 11 • 10 years ago

Comment on attachment 8431363 [details]
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.

	Hema Koka [:hema]
	Comment 12 • 10 years ago

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

	Wilson Page [:wilsonpage] Assignee
	Comment 14 • 10 years ago

(In reply to Jim Porter (:squib) from comment #11)
> Comment on attachment 8431363 [details]
> 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

	Jim Porter (:squib)
	Comment 15 • 10 years ago

Comment on attachment 8476075 [details]
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.

	Jim Porter (:squib)
	Comment 16 • 10 years ago

Oh, and I had one nit on GitHub.

	Wilson Page [:wilsonpage] Assignee
	Comment 17 • 10 years ago

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

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