Yan Or Reporter
	Description • 10 years ago

      No description provided.

	Kevin Grandon :kgrandon
	Comment 2 • 10 years ago

Comment on attachment 8425209 [details]
PR

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

	Wilson Page [:wilsonpage] Assignee
	Comment 4 • 10 years ago

Comment on attachment 8425209 [details]
PR

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

	Yan Or Reporter
	Comment 5 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #2)
> Comment on attachment 8425209 [details]
> 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.

	Yan Or Reporter
	Comment 6 • 10 years ago

(In reply to Wilson Page [:wilsonpage] from comment #4)
> Comment on attachment 8425209 [details]
> 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.

	Jose Manuel Cantera
	Comment 7 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 8 • 10 years ago

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?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 9 • 10 years ago

Comment on attachment 8425209 [details]
PR

Francisco feedback would be more useful than mine here :)

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 10 • 10 years ago

Comment on attachment 8425209 [details]
PR

So removing f?.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 11 • 10 years ago

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

	Francisco Jordano [:arcturus] [:francisco]
	Comment 12 • 10 years ago

Comment on attachment 8425209 [details]
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.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 13 • 10 years ago

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.

	Yan Or Reporter
	Comment 14 • 10 years ago

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.

	Wilson Page [:wilsonpage] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8425209 [details]
PR

Leaving Francisco to take this one

	Arnau March [:arnau] ( not working in Firefox OS anymore :( )
	Comment 16 • 10 years ago

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

	Arnau March [:arnau] ( not working in Firefox OS anymore :( )
	Comment 17 • 10 years ago

Comment on attachment 8425209 [details]
PR

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

	Wilson Page [:wilsonpage] Assignee
	Comment 18 • 10 years ago

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.

	Wilson Page [:wilsonpage] Assignee
	Comment 19 • 10 years ago

CORRECTION (comment 18): *bug 1022866

	Yan Or Reporter
	Comment 20 • 10 years ago

Comment on attachment 8425209 [details]
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.

	Stephany Wilkes
	Comment 21 • 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.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 22 • 10 years ago

Comment on attachment 8425209 [details]
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!

	Wilson Page [:wilsonpage] Assignee
	Comment 23 • 10 years ago

Comment on attachment 8425209 [details]
PR

UPDATED

- Addressed all comments

	Wilson Page [:wilsonpage] Assignee
	Comment 24 • 10 years ago

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)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 25 • 10 years ago

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

	Francisco Jordano [:arcturus] [:francisco]
	Comment 27 • 10 years ago

Comment on attachment 8476579 [details]
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

	Wilson Page [:wilsonpage] Assignee
	Comment 28 • 10 years ago

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

	Francisco Jordano [:arcturus] [:francisco]
	Comment 29 • 10 years ago

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

Great job in here Wilson,

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

	Wilson Page [:wilsonpage] Assignee
	Comment 30 • 10 years ago

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

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

	Anthony Ricaud (:rik)
	Comment 31 • 10 years ago

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

	Francisco Jordano [:arcturus] [:francisco]
	Comment 32 • 10 years ago

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.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 33 • 10 years ago

This is funny,

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