Closed
Bug 1011602
Opened 10 years ago
Closed 10 years ago
[Contacts] Update to use <gaia-header>
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(ux-b2g:2.1)
RESOLVED
FIXED
ux-b2g | 2.1 |
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(2 files, 1 obsolete file)
No description provided.
Updated•10 years ago
|
Blocks: gaia-header
Attachment #8425209 -
Flags: feedback?(wilsonpage)
Attachment #8425209 -
Flags: feedback?(kgrandon)
Attachment #8425209 -
Flags: feedback?(arnau)
Attachment #8425209 -
Flags: feedback?(21)
Comment 2•10 years ago
|
||
Comment on attachment 8425209 [details]
PR
Travis is all kinds of unhappy about this, but the markup looks good!
Attachment #8425209 -
Flags: feedback?(kgrandon)
Assignee | ||
Comment 3•10 years ago
|
||
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]).
(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.
(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.
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 ...
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8425209 -
Flags: feedback?(francisco)
Comment 9•10 years ago
|
||
Comment on attachment 8425209 [details]
PR
Francisco feedback would be more useful than mine here :)
Comment 10•10 years ago
|
||
Comment on attachment 8425209 [details]
PR
So removing f?.
Attachment #8425209 -
Flags: feedback?(21)
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.
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.
Attachment #8425209 -
Flags: feedback?(francisco) → feedback+
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.
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.
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8425209 [details]
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]
PR
Clearing f so you can ask again after closing 2.0 and rebasing the patch.
Thanks Yan!
Attachment #8425209 -
Flags: feedback?(arnau)
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.
Assignee: yor → wilsonpage
Depends on: 1007743
Assignee | ||
Updated•10 years ago
|
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.
Attachment #8425209 -
Flags: review?(francisco)
Attachment #8425209 -
Attachment description: WIP → PR
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.
ux-b2g: --- → 2.1
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!
Attachment #8425209 -
Flags: review?(francisco) → review-
Assignee | ||
Updated•10 years ago
|
Assignee: yor → wilsonpage
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8425209 [details]
PR
UPDATED
- Addressed all comments
Attachment #8425209 -
Flags: review- → review?(francisco)
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)
Flags: needinfo?(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 :(
Flags: needinfo?(francisco)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8425209 -
Attachment is obsolete: true
Attachment #8425209 -
Flags: review?(francisco)
Attachment #8476579 -
Flags: review?(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
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.
Flags: needinfo?(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 :)
Attachment #8476579 -
Flags: review?(francisco) → review+
Flags: needinfo?(francisco)
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8476579 [details] pull-request (master) LANDED https://github.com/mozilla-b2g/gaia/commit/6a296426a159ac5dc59e9de18354762b31071aa9
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 31•10 years ago
|
||
Isn't bug 887541 problematic to land this? Dialer and Messages are waiting on this before landing.
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.
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•