1011602 - [Contacts] Update to use <gaia-header>

Status: RESOLVED FIXED

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

Comment 1

[Yan Or]

Attachment: PR

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8425209 [details] [review]
PR

Travis is all kinds of unhappy about this, but the markup looks good!

Comment 3

[Wilson Page [:wilsonpage]]

Attachment: 1011602.png

Comment 4

[Wilson Page [:wilsonpage]]

Comment on attachment 8425209 [details] [review]
PR

Code looks good, but not able to physically test due to Shadow-DOM platform bug breaking styling (see attachment 8425448 [details]).

Comment 5

[Yan Or]

(In reply to Kevin Grandon :kgrandon from comment #2)
> Comment on attachment 8425209 [details] [review]
> WIP
> 
> Travis is all kinds of unhappy about this, but the markup looks good!

I'll fix the formatting.  I suspect the tests are failing due to the platform issues.

Comment 6

[Yan Or]

(In reply to Wilson Page [:wilsonpage] from comment #4)
> Comment on attachment 8425209 [details] [review]
> WIP
> 
> Code looks good, but not able to physically test due to Shadow-DOM platform
> bug breaking styling (see attachment 8425448 [details]).

Yes, I was able to test some of it in firefox with a combination of inspector and multiple refreshes but will need the platform fixes to give it a full run.

Comment 7

[Jose Manuel Cantera]

Seeing the size of the patch and the new markup I don't see any advantage in what is being proposed. I'm only seeing disadvantages and regression risks. The introduced markup is very similar to the one we already have ...

Comment 8

[Kevin Grandon :kgrandon]

Once we have the new header logic we will definitely want to use web components everywhere due to the tricky amount of JS logic that it will require.

Perhaps we should write some more integration tests to ensure that all of these code paths are covered?

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8425209 [details] [review]
PR

Francisco feedback would be more useful than mine here :)

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8425209 [details] [review]
PR

So removing f?.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jose Manuel Cantera from comment #7)
> Seeing the size of the patch and the new markup I don't see any advantage in
> what is being proposed. I'm only seeing disadvantages and regression risks.
> The introduced markup is very similar to the one we already have ...

Yeah that's the plan for phase 1 of the migration. The idea is to start using Web Components with a minimal amount of changes.

Once those are landed it means that the app should not rely anymore on internals stuffs of the component and we can do deeper changes in a phase 2. The component should also have extensive test coverage to ensure nothing brokes during this phase 2.

In this phase 1, we also want to make sure that the component works everywhere with multiples use cases so we can pinpoint the platform issues and prioritize them to be fixed. It will be painful if at the end we end up with one app using the regular header and all other apps using the Web Component version of it as it means that we will have to support to complete different code paths and won't be able to provide a component + some behavior without regressions because of the lack of tests.

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8425209 [details] [review]
PR

Hi Yan

Thanks a lot for the patch, there are a couple of things:

- The patch needs rebase, but tried directly from your branch and found the same probrlems that Wilson commented.

- Contacts app is sharing part of the code with FTU and there is a set of html pages, for contacts import located in |/shared/pages/import| specially |import.html| and the js used that should also be ported.

- Unit test, it could be nice to add a couple of test to check the actions associated to the headers.

A part from that, the code looks pretty cool, so next round will be quite promissing. 

Don't hesitate to ask again for feedback or review when necessary.

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Hei,

I do agree with Vivien in comment 11.

To me the header has enough complexity and at the same time is small enough to be the perfect candadiate for this.

Also the sooner we start the better we can find problems or improve the platform.

WIN WIN situation, IMHO.

Cheers,
F.

Comment 14

[Yan Or]

Thanks for the feedback, guys.  Lots of changes to the header since this WIP patch so I'll be redoing with added unit tests for review.

Comment 15

[Wilson Page [:wilsonpage]]

Comment on attachment 8425209 [details] [review]
PR

Leaving Francisco to take this one

Comment 16

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Yan, please wait for 2.1 to land this patch :(
We should not compromise Building block header centering work.

Comment 17

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8425209 [details] [review]
PR

Clearing f so you can ask again after closing 2.0 and rebasing the patch.
Thanks Yan!

Comment 18

[Wilson Page [:wilsonpage]]

I heard Yan is away for a while, so I'll take this an update the patch if that's OK. I work in the same office and Francisco so that will help get this landed.

At first glance I'm experiencing bug 1007743.

Comment 19

[Wilson Page [:wilsonpage]]

CORRECTION (comment 18): *bug 1022866

Comment 20

[Yan Or]

Comment on attachment 8425209 [details] [review]
PR

Rebased to latest master and fixed several bugs.

Still looking into a TBPL crash that may not be related but I think we can start the review process.

Comment 21

[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 22

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8425209 [details] [review]
PR

Hi,

thanks a lot for the work, I found a mayor problem with the activity handling, I think cause we have tho elements with same id and the JS code is not modifying the correct one.

A part from that found other visual discrepancies like:
- Settings icon background image is repeating
- Both settings and add contact icons display the words 'settings' and 'add'
- When creating a new contact the 'Done' string while is being disabled (no information added) has a different color.
- While viewing the detail of a contact, the edit icon still displays the word 'edit' underneath

I think that's pretty much all.

Thanks again for the great work!

Comment 23

[Wilson Page [:wilsonpage]]

Comment on attachment 8425209 [details] [review]
PR

UPDATED

- Addressed all comments

Comment 24

[Wilson Page [:wilsonpage]]

I have some failing tests locally, but they seem unrelated. Francisco you have any info on these:

  1) [communications-contacts/test/unit/utilities/image_thumbnail_test.js] Contacts/utilities/thumbnailImage > resizing > should draw the image smaller:
     Uncaught Error: Error: expected 120 to equal 60 (app://communications.gaiamobile.org/common/test/helper.js?time=1408554576312:31)
      at onerror (app://communications.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)


  2) [communications-contacts/test/unit/utilities/image_thumbnail_test.js] Contacts/utilities/thumbnailImage > resizing > should keep the apsect ratio:
     Uncaught Error: Error: expected 120 to equal 60 (app://communications.gaiamobile.org/common/test/helper.js?time=1408554576312:31)
      at onerror (app://communications.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Wilson Page [:wilsonpage] from comment #24)
> I have some failing tests locally, but they seem unrelated. Francisco you
> have any info on these:
> 
>   1) [communications-contacts/test/unit/utilities/image_thumbnail_test.js]
> Contacts/utilities/thumbnailImage > resizing > should draw the image smaller:
>      Uncaught Error: Error: expected 120 to equal 60
> (app://communications.gaiamobile.org/common/test/helper.
> js?time=1408554576312:31)
>       at onerror
> (app://communications.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)
> 
> 
>   2) [communications-contacts/test/unit/utilities/image_thumbnail_test.js]
> Contacts/utilities/thumbnailImage > resizing > should keep the apsect ratio:
>      Uncaught Error: Error: expected 120 to equal 60
> (app://communications.gaiamobile.org/common/test/helper.
> js?time=1408554576312:31)
>       at onerror
> (app://communications.gaiamobile.org/common/vendor/mocha/mocha.js:4959:10)


That sounds weird to me, first time I see those tests failing :(

Comment 26

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 27

[Francisco Jordano [:arcturus] [:francisco]]

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

Hei will, 

nice work, definitely we are getting closer, this are some things I've seen:
- When creating a new contact, the disabled button 'done' has the same color than when it's active.

- When invoked as an activity for picking up a phone, we can see the add and settings icons, and not the close. For testing this try:
Go to sms -> Create new sms -> Pick a contact
We should have a close on the left, and no add or settings.

This case is weird, since we have a similar case and it does work well, the correct case is:
Go to dialer -> enter a number -> press icon add icon -> in the action menu choose add to contact
You'll see how the close button appears correctly and we dont have add or settings

Apart from that LGTM

Comment 28

[Wilson Page [:wilsonpage]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #27)
> - When creating a new contact, the disabled button 'done' has the same color
> than when it's active.

Fixed with latest gaia-header version. Disabled buttons are 50% opacity.

> - When invoked as an activity for picking up a phone, we can see the add and
> settings icons, and not the close. For testing this try:
> Go to sms -> Create new sms -> Pick a contact
> We should have a close on the left, and no add or settings.
> 
> This case is weird, since we have a similar case and it does work well, the
> correct case is:
> Go to dialer -> enter a number -> press icon add icon -> in the action menu
> choose add to contact
> You'll see how the close button appears correctly and we dont have add or
> settings

All of these activity entry points seem to have Close icons for me. Not sure what's different.

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

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

Great job in here Wilson,

let's land this and see what datazilla has to tell us about the performance :)

Comment 30

[Wilson Page [:wilsonpage]]

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

LANDED https://github.com/mozilla-b2g/gaia/commit/6a296426a159ac5dc59e9de18354762b31071aa9

Comment 31

[Anthony Ricaud (:rik)]

Isn't bug 887541 problematic to land this? Dialer and Messages are waiting on this before landing.

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

We have been talking with Wilson and he has asked us about trying to have this QAed before the FL.

It should be blocking not just dialer and messaging but any single app that uses it (several apps using this gaia header component). I've commented in bug 887541 about if this will block our a11 team to continue working, in which case we will backout.

Taking into account the number of code already landed we will need to understand ASAP if bug 887541 is going to land before FL or not.

Comment 33

[Francisco Jordano [:arcturus] [:francisco]]

This is funny,

I would like to keep the depends bug, since if it doesn't land, we will backout this changes.