1015297 - [Settings] Update to use gaia-header

Status: RESOLVED FIXED

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

Comment 1

[Wilson Page [:wilsonpage]]

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.

Comment 2

[Yan Or]

Yes, I've got something already.  Will send out the PR next week.

Comment 3

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 4

[Wilson Page [:wilsonpage]]

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.

Comment 5

[Wilson Page [:wilsonpage]]

[1] http://github.com/gaia-components/gaia-icons

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

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?

Comment 7

[Wilson Page [:wilsonpage]]

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

Comment 8

[Wilson Page [:wilsonpage]]

I found bug 1052557 in the process which is preventing me from fully testing the SIM Manager panel.

Comment 9

[Wilson Page [:wilsonpage]]

Arthur, can you take another look at this?

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 11

[Wilson Page [:wilsonpage]]

(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.

Comment 12

[Stephany Wilkes]

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.

Comment 13

[Stephany Wilkes]

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!

Comment 14

[Gregor Wagner [:gwagner]]

This blocks now the whole haida feature. We need to resolve this today!

Comment 15

[Stephany Wilkes]

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

[Hema Koka [:hema]]

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

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

EJ, per the discussion earlier, please help check the patch, thanks!

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !

Comment 20

[Wilson Page [:wilsonpage]]

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

UPDATED

- Github comments addressed
- Patch left un-squashed for quick review

Comment 21

[Stephany Wilkes]

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 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 ?

Comment 23

[Wilson Page [:wilsonpage]]

(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.

Comment 24

[Wilson Page [:wilsonpage]]

According to comment 16, this patch bust land after bug 938177.

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 :)

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Let's keep observing whether there is significant performance problem in gaia-header or not .

Comment 27

[Wilson Page [:wilsonpage]]

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

LANDED https://github.com/mozilla-b2g/gaia/commit/888cb83588d90e512904d34a30a9353f2f010bd6

Comment 28

[Coler]

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!

Comment 29

[Yan Or]

Getting rid of this old ni? to clear my queue.