810237 - [Homescreen] BB deleting apps

Status: RESOLVED FIXED

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

Description

[Cristian Rodriguez (:crdlc)]

http://img10.imageshack.us/img10/2135/deleteapp.png
http://img819.imageshack.us/img819/2765/removebookmark.png

Comment 1

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/pull/6319

Comment 2

[Cristian Rodriguez (:crdlc)]

Attachment: The patch

Comment 3

[Cristian Rodriguez (:crdlc)]

patch is true? sorry, what does it mean? thanks Vivien

Comment 4

[Cristian Rodriguez (:crdlc)]

ok I understand, I forgot to click on patch checkbox. Thanks

Comment 5

[David Scravaglieri [:scravag]]

Is this feature work or applying BB ?

Comment 6

[Cristian Rodriguez (:crdlc)]

applying BB

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 680032 [details] [diff] [review]
The patch

Review of attachment 680032 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/homescreen/js/request.js
@@ +21,5 @@
> +    */
> +    show: function dialog_show(title, msg, cancel, confirm) {
> +      screen = document.createElement('form');
> +      screen.setAttribute('role', 'dialog');
> +      screen.dataset.type = 'confirm';

I thought the building blocks was about inlining the html directly?

Let's redirect this review to Etienne since he works on it last week.

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 680032 [details] [diff] [review]
The patch

Review of attachment 680032 [details] [diff] [review]:
-----------------------------------------------------------------

Recaping: the idea is to put the dialog in the index.html directly and just toggle the display.
I think the homescreen is a great candidate for this since we have only 1 type of dialog to handle (the one displayed when removing an app/bookmark).

It will probably end up being a simpler patch too! :)

::: apps/homescreen/js/request.js
@@ -35,5 @@
> -        screen.id = 'permission-screen';
> -
> -        dialog = document.createElement('div');
> -        dialog.id = 'permission-dialog';
> -        screen.appendChild(dialog);

Yes the plan discussed with Ismael was to fade out CustomDialog in favor inlined BB-compliant markup + a simple css class toggle to show/hide.

Comment 9

[Cristian Rodriguez (:crdlc)]

perfect, got it, thanks friends

Comment 10

[Cristian Rodriguez (:crdlc)]

Attachment: confirm in html markup

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 682383 [details]
confirm in html markup

Looking good!

r=me with the 2 nits listed on github addressed.
(The renaming of CustomDialog -> UninstallDialog is the most important)

Please squash/amend the commit with the right reviewer.

Comment 12

[Cristian Rodriguez (:crdlc)]

done both tasks :)

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to crdlc from comment #12)
> done both tasks :)

Awesome thanks!

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 682383 [details]
confirm in html markup

It sounds a really sane code that removes a custom helper in Javascript to use the one from the building blocks which is definitively simpler.

a=me.

Comment 15

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/cf233387c8a286800f4fee9e06d32d2bb093a45e