Closed Bug 1015297 Opened 8 years ago Closed 8 years ago

[Settings] Update to use gaia-header

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(ux-b2g:2.1, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S3 (29aug)
ux-b2g 2.1
Tracking Status
b2g-v2.1 --- fixed

People

(Reporter: yor, Assigned: wilsonpage)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
eragonj
: review+
Details | Review
No description provided.
Blocks: gaia-header
Assignee: nobody → yor
Status: NEW → ASSIGNED
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.
Attached file pull-request (master)
Comment on attachment 8468454 [details] [review]
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 on attachment 8468454 [details] [review]
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)
Blocks: 1036394
Comment on attachment 8468454 [details] [review]
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)
See Also: → 1052557
I found bug 1052557 in the process which is preventing me from fully testing the SIM Manager panel.
Arthur, can you take another look at this?
Flags: needinfo?(arthur.chen)
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)
(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)
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
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)
Blocks: 938177
This blocks now the whole haida feature. We need to resolve this today!
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).
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 on attachment 8468454 [details] [review]
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)
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] [review]
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)
Comment on attachment 8468454 [details] [review]
pull-request (master)

UPDATED

- Github comments addressed
- Patch left un-squashed for quick review
Attachment #8468454 - Flags: review?(ejchen)
Flags: needinfo?(wilsonpage)
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] [review]
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)
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #22)
> Comment on attachment 8468454 [details] [review]
> 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.
Depends on: 1055004
No longer blocks: 938177
Attachment #8468454 - Flags: review?(ejchen)
According to comment 16, this patch bust land after bug 938177.
Depends on: 938177
Comment on attachment 8468454 [details] [review]
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 .
Depends on: 887541
No longer depends on: 887541
Depends on: 887541
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1060208
See Also: → 1060295
Depends on: 1062007
Target Milestone: --- → 2.1 S3 (29aug)
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)
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.