[Contacts][NGA] Remove overlay dependencies with contacts.js

RESOLVED FIXED in Firefox OS master

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mancas, Assigned: mancas)

Tracking

unspecified
FxOS-S8 (02Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

(Whiteboard: [NG Gaia Contacts][patch][only for NGA Contacts branch])

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
Some parts of the contacts app, shows and hides the overlay by calling a function that lives in contacts.js. A better approach could be by throwing events in order to notice the app when the overlay is shown/hidden and then perform any operation related to the contacts list.

Using these events, we can rid off dependencies with Contacts, in a lot of js files
Created attachment 8629887 [details] [review]
[gaia] mancas:bug1180627 > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
Created attachment 8629888 [details] [review]
Patch
Attachment #8629888 - Flags: feedback?(borja.bugzilla)
Target Milestone: --- → FxOS-S2 (10Jul)
Comment on attachment 8629888 [details] [review]
Patch

Some comments in Github. In this case the approach based in events collide with the separation in views, due to 'List' and 'Settings' will be, potentially, self-contained views (so one has no knowledge about the other). I would move to a Promise based approach, so when deletion is done we can do what we need.

Keep in mind that this is a tough scenario, due to in our proposal we will have something as:
list.html > settings.html (tap in 'delete contacts') > list.html?edit=true > list.html

So we need to review where this 'overlay' should be executed. We could talk offline about this, because seems to be a scenario for testing that the NGA is ready, and all scenarios are covered as expected. Thanks!
Attachment #8629888 - Flags: feedback?(borja.bugzilla)
Target Milestone: FxOS-S2 (10Jul) → FxOS-S3 (24Jul)
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
(Assignee)

Updated

3 years ago
Attachment #8629888 - Flags: review?(borja.bugzilla)
Comment on attachment 8629888 [details] [review]
Patch

As we want to load just the resources we need, and remove all panels not needed when used, I would modify [1] for lazy loading the panels we need (in this case overlay), and destroy the markup when done. Address this and ask me to review this, and we are done!


[1] https://github.com/mozilla-b2g/gaia/pull/30836/files#diff-efca3f318e7eb8a48336855e8a8a958bR7
Attachment #8629888 - Flags: review?(borja.bugzilla) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8629888 - Flags: review?(borja.bugzilla)
Attachment #8629888 - Flags: review?(borja.bugzilla) → review+
(Assignee)

Comment 5

3 years ago
Landed in master: https://github.com/mozilla-b2g/gaia/commit/00900a0f9e20694da0a770c4fb4c138c96dcc937
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-b2g-master: --- → fixed
We found some issues, so let's revert it and wait till fix them!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1192174
Target Milestone: FxOS-S4 (07Aug) → FxOS-S5 (21Aug)
Created attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master
(Assignee)

Comment 8

3 years ago
Comment on attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master

Hey Borja could you take a look at the new approach?
Attachment #8645647 - Flags: feedback?(borja.bugzilla)
Comment on attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master

I would suggest to change the approach, and refactor this overlay. I would remove the namespace of `window.utils`, and I would create it as a plug&play element (no classes/styles from outside).
Attachment #8645647 - Flags: feedback?(borja.bugzilla)
Blocks: 1169191
(Assignee)

Comment 10

3 years ago
Comment on attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master

Hey Francisco, could you review the new overlay when you get a chance?

Thank you!
Attachment #8645647 - Flags: review?(francisco)
Comment on attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master

I've added some comments in the PR. Mainly I would like to expose a simple interface (show, hide....) instead of letting the user to define params as 'type' from outside (due to actually it's a 'String').

Let me know when ready and I'll review this. Thanks!
Attachment #8645647 - Flags: review?(francisco)
(Assignee)

Updated

3 years ago
Attachment #8645647 - Flags: review?(borja.bugzilla)
Target Milestone: FxOS-S5 (21Aug) → FxOS-S6 (04Sep)
Comment on attachment 8645647 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:master

Just fix the hint errors and we are done!
Attachment #8645647 - Flags: review?(borja.bugzilla) → review+
Target Milestone: FxOS-S6 (04Sep) → FxOS-S7 (18Sep)
Target Milestone: FxOS-S7 (18Sep) → ---
Created attachment 8663623 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:nga
(Assignee)

Comment 14

3 years ago
Comment on attachment 8663623 [details] [review]
[gaia] mancas:overlay > mozilla-b2g:nga

New PR against NGA branch
Attachment #8663623 - Flags: review+
Target Milestone: --- → FxOS-S8 (02Oct)
Whiteboard: [NG Gaia Contacts][patch] → [NG Gaia Contacts][patch][only for NGA Contacts branch]
Blocks: 1190859
Blocks: 1190845
(Assignee)

Comment 15

3 years ago
Landed in master https://github.com/mozilla-b2g/gaia/commit/b2356784b18b9241057d7f20ade3fc35b82d06ae
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
I hope that you mean 'landed in nga' :)
(Assignee)

Comment 17

3 years ago
Yeah! The path has been landed in nga not in master =P
You need to log in before you can comment on or make changes to this bug.