Closed
Bug 1015248
Opened 10 years ago
Closed 10 years ago
[Video] Update to use gaia-header
Categories
(Firefox OS Graveyard :: Gaia::Video, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(4 files)
No description provided.
Blocks: gaia-header
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8430874 -
Flags: review?(yor)
Attachment #8430874 -
Flags: review?(dflanagan)
Attachment #8430874 -
Flags: review?(arnau)
Attachment #8430874 -
Flags: review?(yor) → review+
Please Wilson, wait for 2.1 to land this patch :(
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.
Attachment #8430874 -
Flags: review?(dflanagan) → review?(rnicoletti)
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.
Attachment #8430874 -
Flags: review?(rnicoletti) → review+
Comment on attachment 8430874 [details]
pull-request (master)
clearing review as it has been reviewed by the app peers and Yan :)
Attachment #8430874 -
Flags: review?(arnau)
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8430874 -
Flags: review?(kgrandon)
Attachment #8430874 -
Flags: feedback?
Assignee | ||
Updated•10 years ago
|
Attachment #8430874 -
Flags: feedback?
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?
Attachment #8430874 -
Flags: review?(kgrandon) → review-
Assignee | ||
Updated•10 years ago
|
Attachment #8430874 -
Flags: review- → review?(kgrandon)
Assignee | ||
Comment 9•10 years ago
|
||
UPDATE: External components are now installed into `shared/elements/<package-name>`
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.
Attachment #8430874 -
Flags: review?(kgrandon) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Waiting for icon-font solution to land to avoid start-up regression here.
Depends on: 1036394
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.
Attachment #8430874 -
Flags: review+ → review?(rnicoletti)
Comment 15•10 years ago
|
||
Hi Wilson, some functionality is broken by the PR. I've updated the PR with my comments.
Flags: needinfo?(rnicoletti)
Comment 16•10 years ago
|
||
Comment on attachment 8430874 [details]
pull-request (master)
r- for broken functionality.
Attachment #8430874 -
Flags: review?(rnicoletti) → review-
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.
Attachment #8430874 -
Flags: review- → review?(rnicoletti)
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
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.
Flags: needinfo?(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.
Flags: needinfo?(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.
Comment 23•10 years ago
|
||
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?
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
Comment 26•10 years ago
|
||
Comment on attachment 8430874 [details]
pull-request (master)
Looks good.
Attachment #8430874 -
Flags: review?(rnicoletti) → review+
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
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8430874 [details] pull-request (master) LANDED https://github.com/mozilla-b2g/gaia/commit/18b47f3f8fab155cb03905528bbf4e67b9bea07f
Assignee | ||
Updated•10 years ago
|
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
•