[Homescreen] BB deleting apps

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: crdlc, Assigned: crdlc)

Tracking

({polish})

unspecified
x86
Gonk (Firefox OS)
polish

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Updated

6 years ago
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
blocking-basecamp: --- → ?
(Assignee)

Comment 2

6 years ago
Created attachment 680032 [details] [diff] [review]
The patch
Attachment #680032 - Flags: review?(21)
(Assignee)

Comment 3

6 years ago
patch is true? sorry, what does it mean? thanks Vivien
(Assignee)

Comment 4

6 years ago
ok I understand, I forgot to click on patch checkbox. Thanks
Is this feature work or applying BB ?
Flags: needinfo?(crdlc)
Keywords: polish
(Assignee)

Comment 6

6 years ago
applying BB
Flags: needinfo?(crdlc)
blocking-basecamp: ? → -
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.
Attachment #680032 - Flags: review?(21) → review?(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.
(Assignee)

Comment 9

6 years ago
perfect, got it, thanks friends
(Assignee)

Comment 10

6 years ago
Created attachment 682383 [details]
confirm in html markup
Attachment #682383 - Flags: review?(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.
Attachment #682383 - Flags: review?(etienne) → review+
Attachment #680032 - Attachment is obsolete: true
Attachment #680032 - Flags: review?(etienne)
(Assignee)

Comment 12

6 years ago
done both tasks :)
(Assignee)

Updated

6 years ago
Attachment #682383 - Flags: approval-gaia-master?(21)
(In reply to crdlc from comment #12)
> done both tasks :)

Awesome thanks!
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.
Attachment #682383 - Flags: approval-gaia-master?(21) → approval-gaia-master+
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.