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

Status: RESOLVED FIXED

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

Description

[Manuel Casas Barrado [:mancas]]

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

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mancas:bug1180627 > mozilla-b2g:master

Comment 2

[Manuel Casas Barrado [:mancas]]

Attachment: Patch

Comment 3

[Borja Salguero [:borjasalguero]]

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!

Comment 4

[Borja Salguero [:borjasalguero]]

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

Comment 5

[Manuel Casas Barrado [:mancas]]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/00900a0f9e20694da0a770c4fb4c138c96dcc937

Comment 6

[Borja Salguero [:borjasalguero]]

We found some issues, so let's revert it and wait till fix them!

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] mancas:overlay > mozilla-b2g:master

Comment 8

[Manuel Casas Barrado [:mancas]]

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

Hey Borja could you take a look at the new approach?

Comment 9

[Borja Salguero [:borjasalguero]]

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

Comment 10

[Manuel Casas Barrado [:mancas]]

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!

Comment 11

[Borja Salguero [:borjasalguero]]

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!

Comment 12

[Borja Salguero [:borjasalguero]]

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

Just fix the hint errors and we are done!

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] mancas:overlay > mozilla-b2g:nga

Comment 14

[Manuel Casas Barrado [:mancas]]

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

New PR against NGA branch

Comment 15

[Manuel Casas Barrado [:mancas]]

Landed in master https://github.com/mozilla-b2g/gaia/commit/b2356784b18b9241057d7f20ade3fc35b82d06ae

Comment 16

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

I hope that you mean 'landed in nga' :)

Comment 17

[Manuel Casas Barrado [:mancas]]

Yeah! The path has been landed in nga not in master =P