Closed Bug 1011602 Opened 9 years ago Closed 8 years ago

[Contacts] Update to use <gaia-header>

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(ux-b2g:2.1)

RESOLVED FIXED
ux-b2g 2.1

People

(Reporter: yor, Assigned: wilsonpage)

References

Details

Attachments

(2 files, 1 obsolete file)

22.73 KB, image/png
Details
46 bytes, text/x-github-pull-request
arcturus
: review+
Details | Review
No description provided.
Attached file PR (obsolete) —
Attachment #8425209 - Flags: feedback?(wilsonpage)
Attachment #8425209 - Flags: feedback?(kgrandon)
Attachment #8425209 - Flags: feedback?(arnau)
Attachment #8425209 - Flags: feedback?(21)
Assignee: nobody → yor
Status: NEW → ASSIGNED
Comment on attachment 8425209 [details] [review]
PR

Travis is all kinds of unhappy about this, but the markup looks good!
Attachment #8425209 - Flags: feedback?(kgrandon)
Attached image 1011602.png
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]).
(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.
(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.
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 ...
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?
Attachment #8425209 - Flags: feedback?(francisco)
Comment on attachment 8425209 [details] [review]
PR

Francisco feedback would be more useful than mine here :)
(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 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.
Attachment #8425209 - Flags: feedback?(francisco) → feedback+
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.
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.
Status: ASSIGNED → NEW
Comment on attachment 8425209 [details] [review]
PR

Leaving Francisco to take this one
Attachment #8425209 - Flags: feedback?(wilsonpage)
Yan, please wait for 2.1 to land this patch :(
We should not compromise Building block header centering work.
Comment on attachment 8425209 [details] [review]
PR

Clearing f so you can ask again after closing 2.0 and rebasing the patch.
Thanks Yan!
Attachment #8425209 - Flags: feedback?(arnau)
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.
Assignee: yor → wilsonpage
Depends on: 1007743
Depends on: 1022866
No longer depends on: 1007743
CORRECTION (comment 18): *bug 1022866
No longer depends on: 1022866
Depends on: 1022866
Assignee: wilsonpage → yor
Status: NEW → ASSIGNED
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.
Attachment #8425209 - Flags: review?(francisco)
Attachment #8425209 - Attachment description: WIP → PR
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 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!
Attachment #8425209 - Flags: review?(francisco) → review-
Assignee: yor → wilsonpage
Comment on attachment 8425209 [details] [review]
PR

UPDATED

- Addressed all comments
Attachment #8425209 - Flags: review- → review?(francisco)
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)
Flags: needinfo?(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 :(
Flags: needinfo?(francisco)
Attached file pull-request (master)
Attachment #8425209 - Attachment is obsolete: true
Attachment #8425209 - Flags: review?(francisco)
Attachment #8476579 - Flags: review?(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
(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.
Flags: needinfo?(francisco)
Depends on: 1055004
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 :)
Attachment #8476579 - Flags: review?(francisco) → review+
Flags: needinfo?(francisco)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Isn't bug 887541 problematic to land this? Dialer and Messages are waiting on this before landing.
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.
Depends on: 887541
No longer depends on: 887541
Depends on: 887541
This is funny,

I would like to keep the depends bug, since if it doesn't land, we will backout this changes.
You need to log in before you can comment on or make changes to this bug.