Closed
Bug 1015297
Opened 10 years ago
Closed 10 years ago
[Settings] Update to use gaia-header
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(ux-b2g:2.1, b2g-v2.1 fixed)
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | fixed |
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(1 file)
No description provided.
Blocks: gaia-header
Assignee | ||
Comment 1•10 years ago
|
||
There is no PR for this yet, so going to get cracking on one. Yan give us a shout if you've already got something brewing.
Assignee: yor → wilsonpage
Yes, I've got something already. Will send out the PR next week.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8468454 [details] pull-request (master) arthurcc: I saw that you reviewed bug 948856, so thought you might be suitable to review this one. - Updated all view headers to <gaia-header> - Updated to use the official gaia-icons [1] font that ships with gaia-header. This uses the new file based @font-face which is required to benefit from the performance improvements landing as part of bug 1036394. - Switched out some more PNGs that weren't switched in bug 948856.
Attachment #8468454 -
Flags: review?(arthur.chen)
Comment 6•10 years ago
|
||
Comment on attachment 8468454 [details]
pull-request (master)
Thanks for the effort, Wilson. Overall the patch looks great. I left some comments in github regarding some minor issues, please have a look. And I noticed that sometimes there is no response when clicking on the back button, is this a known issue of gaia-header?
Attachment #8468454 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8468454 [details]
pull-request (master)
UPDATED PATCH
- Used action="back" API wherever possible to make sure that we get the benefits of the encapsulated action button. I didn't want to touch the more complex type="reset" and type="submit" logic, so some panes don't use the action="back" approach.
- Addressed Github comments
Attachment #8468454 -
Flags: review?(arthur.chen)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 8•10 years ago
|
||
I found bug 1052557 in the process which is preventing me from fully testing the SIM Manager panel.
Assignee | ||
Comment 9•10 years ago
|
||
Arthur, can you take another look at this?
Flags: needinfo?(arthur.chen)
Comment 10•10 years ago
|
||
Is this a committed feature for v2.1? I am currently working on some 2.1 features so will probably check this patch later. I need some time to check all of the workflows.
Flags: needinfo?(arthur.chen)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #10) > Is this a committed feature for v2.1? I am currently working on some 2.1 > features so will probably check this patch later. I need some time to check > all of the workflows. Yes this is required for v2.1 as we are deprecating the headers.css building-block.
Flags: needinfo?(arthur.chen)
Comment 12•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 13•10 years ago
|
||
Team, This review flag has not moved since August 6. 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. Arthur, please reassign or review (+ Tim to advise on reassignment if needed) and let me know if anything else is blocking here. Thanks!
Flags: needinfo?(timdream)
Comment 15•10 years ago
|
||
Arthur and Tim, please get to this as soon as you can in the morning of Wed., your time. I tried for hours today with Candice, Fabrice and Kyle to find another reviewer to start this in either Europe or anywhere in the U.S. and Canada, and I can't find anyone else who might be able to take it sooner (sorry Gregor).
Comment 16•10 years ago
|
||
Wilson/Yan, Since it will take some time to get the gaia-header integration for settings reviewed, I would not couple it with a must-have feature for the systems fe team for this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=938177 Once Gregor's team land their code, please merge and land your header changes after r+/risk assessment of this patch. Thanks Hema
Flags: needinfo?(wilsonpage)
Comment 17•10 years ago
|
||
Comment on attachment 8468454 [details]
pull-request (master)
EJ, per the discussion earlier, please help check the patch, thanks!
Attachment #8468454 -
Flags: review?(arthur.chen) → review?(ejchen)
Flags: needinfo?(timdream)
Flags: needinfo?(arthur.chen)
Comment 18•10 years ago
|
||
I basically agree with Hema; The dependency is only flagged on bug 938177 comment 43, 9 hours ago. Since this bug is ux-b2g: 2.1 and not feature-b2g: 2.1, I am not going to push my developers to land it (if the current patch is unfit) just to unblock. To unblock we really need to use the old icons.css way since that's what we have for 2.1. That said, Wilson can turn the table around if the patch we are looking at can be merged in a day or two.
Comment on attachment 8468454 [details]
pull-request (master)
Hi Wilson,
I just left some comments on Github and there are some problems we have to fix. Please go check it and ping me if you have any problem.
One more thing, please try to rebase first before setting a review flag because I want CI to help us catch some missing flaws when reviewing. Thanks !
Attachment #8468454 -
Flags: review?(ejchen)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8468454 [details]
pull-request (master)
UPDATED
- Github comments addressed
- Patch left un-squashed for quick review
Attachment #8468454 -
Flags: review?(ejchen)
Flags: needinfo?(wilsonpage)
Comment 21•10 years ago
|
||
The ux flag is simply to mark features that required UX work. This was not blocking on UX; it was blocking Gregor (not a UX issue). I looked for reviewers after Gregor flagged it, even though web components is really not in the UX wheelhouse. You don't need to push anyone to land for 2.1. We're just trying to get an assessment of how the web components bugs are looking to create an accurate idea of which will or will not land. Thanks Tim!
Comment on attachment 8468454 [details]
pull-request (master)
Thanks Wilson ! this patch looks awesome now. Just left some comments on github about runHeaderFontFit() and please fix it !
And here comes a small question, I am not sure it happens there or not, but after applying this patch, it takes a little bit longer to open the panel, is there any performance impact in gaia-header ?
Attachment #8468454 -
Flags: review?(ejchen)
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #22) > Comment on attachment 8468454 [details] > pull-request (master) > > Thanks Wilson ! this patch looks awesome now. Just left some comments on > github about runHeaderFontFit() and please fix it ! Fixed > And here comes a small question, I am not sure it happens there or not, but > after applying this patch, it takes a little bit longer to open the panel, > is there any performance impact in gaia-header ? I was curious too so I ran a simple pane (device information) side-by-side (patch vs. master) and couldn't see any noticeable difference. The font-fit logic does cause one reflow, but this is the case for master too.
Assignee | ||
Updated•10 years ago
|
Attachment #8468454 -
Flags: review?(ejchen)
Assignee | ||
Comment 24•10 years ago
|
||
According to comment 16, this patch bust land after bug 938177.
Depends on: 938177
Comment on attachment 8468454 [details]
pull-request (master)
Hi Wilson,
Thanks for all your hard works, please make sure CI is green before merging codes !
I really appreciate it :)
Attachment #8468454 -
Flags: review?(ejchen) → review+
Let's keep observing whether there is significant performance problem in gaia-header or not .
Assignee | ||
Comment 27•10 years ago
|
||
Comment on attachment 8468454 [details] pull-request (master) LANDED https://github.com/mozilla-b2g/gaia/commit/888cb83588d90e512904d34a30a9353f2f010bd6
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
status-b2g-v2.1:
--- → fixed
Target Milestone: --- → 2.1 S3 (29aug)
Comment 28•10 years ago
|
||
Hi Yan, Could you provide STRs or video to help us verify this issue? If no str or video, can we verify it woth https://bugzilla.mozilla.org/show_bug.cgi?id=1052557? thanks!
Flags: needinfo?(yor)
Reporter | ||
Comment 29•7 years ago
|
||
Getting rid of this old ni? to clear my queue.
Flags: needinfo?(yor)
You need to log in
before you can comment on or make changes to this bug.
Description
•