Yan Or Reporter
	Description • 10 years ago

      No description provided.

	Yan Or Reporter
	Comment 2 • 10 years ago

Looks good.  Travis is failing though.

	Arnau March [:arnau] ( not working in Firefox OS anymore :( )
	Comment 3 • 10 years ago

Please Wilson, wait for 2.1 to land this patch :(

	David Flanagan [:djf]
	Comment 4 • 10 years ago

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

Russ,

Could you take this review for me, please? I'm running out of time to review things before the FL date.

	Russ Nicoletti [:russn]
	Comment 5 • 10 years ago

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

r+, but there some is cleanup I've mentioned in the PR that would be good to take care of before landing. Also, the PR is out of date, can't be merged as it is.

	Arnau March [:arnau] ( not working in Firefox OS anymore :( )
	Comment 6 • 10 years ago

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

clearing review as it has been reviewed by the app peers and Yan :)

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

The v2.0 header refresh stuff has landed and has large conflicts with this patch. I think we should put this on hold until the updated version on <gaia-header> is ready.

	Kevin Grandon :kgrandon
	Comment 8 • 10 years ago

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

After taking a look at the new approach (changed since original R+s), I don't think I'm too happy with it.

My main concerns: It seems that we would have *three* different ways of defining and implementing web components in 2.1. I don't think this is acceptable. This is going to cause way too much confusion, and not send a clear message to developers. I still don't think having two different approaches is a good idea either, but like we said, it might be ok to experiment with using another approach than shared/elements at small scale. We should either backport your component into elements/gaia_header, or move all of the existing elements into shared/components/.

If the reason for not doing this is because of icons, perhaps those could go into another shared/ folder? Or bower install into two different directories?

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

UPDATE: External components are now installed into `shared/elements/<package-name>`

	Kevin Grandon :kgrandon
	Comment 10 • 10 years ago

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

This is still not my preferred approach, as the code isn't inline with the other components, but I think it's fine to try for now while we get some more data.

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

Waiting for icon-font solution to land to avoid start-up regression here.

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

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

Hey Russ, this is going to need a second review from someone close to the app after a few changes.

	Russ Nicoletti [:russn]
	Comment 13 • 10 years ago

Hi Wilson, I'll take a look when I'm back from vacation on 8/6.

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

Any update on this?

	Russ Nicoletti [:russn]
	Comment 15 • 10 years ago

Hi Wilson, some functionality is broken by the PR. I've updated the PR with my comments.

	Russ Nicoletti [:russn]
	Comment 16 • 10 years ago

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

r- for broken functionality.

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

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

Fixed. I'm not sure how that happened, perhaps a poorly resolved conflict.

I have this patch sitting on top of the patch for bug 1050801 as it depends on the new gaia-header. Once bug 1050801 lands, I'll rebase this patch against master and land.

	Russ Nicoletti [:russn]
	Comment 20 • 10 years ago

Hi Wison, I haven't finished the review but I have added some comments to the PR regarding some other issues I've found. I'm using NI in case you're not reading your bug mail.

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

(In reply to Russ Nicoletti [:russn] from comment #19)
> Created attachment 8470359 [details]
> 'save' button after applying patch

If you update to latest Gecko this layout bug will be fixed.

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

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

I think all the weirdness you saw is a result of out-of-date Gecko. I've also changed so that the 'Save' button is shown after the storage availability callback.

	Russ Nicoletti [:russn]
	Comment 24 • 10 years ago

(In reply to Wilson Page [:wilsonpage] from comment #22)
> Comment on attachment 8430874 [details]
> pull-request (master)
> 
> I think all the weirdness you saw is a result of out-of-date Gecko. I've
> also changed so that the 'Save' button is shown after the storage
> availability callback.

Yes, the weirdness went away when I upgraded to the latest gecko (sorry for the confusion). The one issue I'm still seeing is the 'options' button on the pick header. Are you also seeing that? Is that intentional?

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

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

Updated patch:

- Added `!important` (sorry) to override strong `<style scoped>` selectors

	Russ Nicoletti [:russn]
	Comment 26 • 10 years ago

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

Looks good.

	Russ Nicoletti [:russn]
	Comment 27 • 10 years ago

Fyi, you'll need to update build/csslint/xfail.list to reflect the additional '!important' warnings in video.css and video_tablet.css

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

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

LANDED https://github.com/mozilla-b2g/gaia/commit/18b47f3f8fab155cb03905528bbf4e67b9bea07f