[gallery] [BB] Delete items and photos confirmation should have class=danger

RESOLVED FIXED

Status

Firefox OS
Gaia::Gallery
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pabloUX, Unassigned)

Tracking

({polish})

unspecified
All
Gonk (Firefox OS)
polish

Firefox Tracking Flags

(blocking-basecamp:-)

Details

(Whiteboard: visual)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Delete confirmations should prevent the user he/she is going to do a "dangerous" action, so it BB class should be danger (it changes the color of the button to red). Label in the button should be "Delete" not Yes.
(Reporter)

Updated

5 years ago
Keywords: polish
Whiteboard: visual
(Reporter)

Updated

5 years ago
blocking-basecamp: --- → ?
should not block on polish
blocking-basecamp: ? → -
Created attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

At the moment the normal confirm() dialog is used to get confirmation for deletion of photo's in the gallery. This patch replaces it with the standard HTML/CSS dialog, adds the right text to it and adds a danger class to the delete button.

In terms of localisation, the Delete term is used in a variety of places, do I need to really add it here as well for all locales or do we have a shared way for terms like this?
Attachment #722260 - Flags: review?(dflanagan)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

Cancelling the review for now.

I don't know who the bug reporter is and whether they speak with any authority about this. I'm blissfully unaware of what our UX guidelines are here, so I'm asking for feedback from Casey and/or Patryk on this.  Is the reporter correct that the button should be red and read "Delete" instead of Ok?

Jan: I haven't tested your patch, but it seems okay to me. Instead of making it so specific to file deletion, though, I'd challenge you to write a generic function in a new file shared/js file.  Maybe something like this:

function async_confirm(msg, yesText, noText, isDangerous, callback)

Some of our BB code is sort of useless without supporting JS, and this is an opportunity to create generically useful support functions.  (You could look to see how other apps are using that BB CSS and write a function that could be used in those apps as well.)
Attachment #722260 - Flags: review?(dflanagan)
Attachment #722260 - Flags: feedback?(padamczyk)
Attachment #722260 - Flags: feedback?(kyee)
David: yeah, I've thought about that but I think that the building block for confirm already does have the flexibility that you'd normally add in the API. Think: what is the text, what is the title (I omitted that one here but there can be one), which buttons do we have here, what do they read, which classes do they have. On top of that the confirm css code needs to be loaded into the page. If we're abstracting that away we'll end up with recreating HTML in javascript which doesn't really add any value there. If we need lesser abstractions there we should rather fix that in the building blocks than creating boilerplate.

Anyway, I do agree on the proposed patch. The danger class is required for all actions that have a permanent dangerous effect, like deleting photo's. Other than that, the 'OK' button is just because the 'confirm()' was used here, so there wasn't a real thought process before that.
Jan,

What would you do if the gallery app required two different confirmation dialogs with different messages? I assume you'd write a utility function so you could reuse one set of HTML elements, right?  That's all I'm suggesting you do here.
David, If they are completely the same then yes, you can easily abstract that. However, you're simplifying the scenario here. There are a bunch of scenario's where you can use the confirm box, so adding the proposed interface is only supporting a few of them, which in the end will make you re-implement the confirm building block, but this time in javascript instead of HTML/CSS.

Stated otherwise: if an app has two header elements, for two different views, would you write a utility method to reuse one set of HTML elements?
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

Can you please attach a screenshot?
Created attachment 725328 [details]
Screenshot of the change

I've attached a screenshot. The render bug on the right side is something that happens for all confirmation screens in FF desktop. Doesn't happen on the device.
Flags: needinfo?(padamczyk)

Comment 9

5 years ago
(In reply to janjongboom from comment #4)
> <snip> The danger class is required for
> all actions that have a permanent dangerous effect, like deleting photo's.<snip>

Yes this is correct.   Screen shot looks good.

Pablo is part of the Telefonica UX team and is helping identify and inconsistencies in the UX.
Flags: needinfo?(padamczyk)
Alright, can we merge this?
(In reply to janjongboom from comment #10)
> Alright, can we merge this?

Looks good to me.
Attachment #722260 - Flags: feedback?(padamczyk) → feedback+
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

Patryk and Casey: thanks for clarifying the UX requirements for me.  I'm clearing the feedback request for Casey, since he provided it.

Jan: I'm also setting review- on the patch as it stands. I don't like the way it currently mixes the dialog show/hide logic with the file deletion logic.

Please write a replacement function for confirm() and use that in the deleteSelectedItems() instead.  I imagine it would be something like confirm(message, buttonLabel, isDangerous, onConfirmCallback). I think this would be a useful thing to have in the shared/js/ directory, but its okay with me if you want to just put it in gallery/js/confirm.js or just add it to gallery.js directly. Even better if you lazy load confirm.css so we don't take a hit on startup time.

Then, in the HTML file, change the element ids to remove the 'delete-' prefix so the dialog is not specific to file deletion.
Attachment #722260 - Flags: feedback?(kyee) → review-
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

I have adjusted the PR regarding your last commit. Please re-review. I have not squashed the commits yet so we can go back if we'd like, I'll squash after a r+.
Attachment #722260 - Flags: review- → review?(dflanagan)
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

Jan,

I should have been clearer in my last comment about the lazy loader.  The gallery app already loads shared/js/lazy_loader.js and that module handles CSS lazy loading, so please use that instead.

I've made some other requests and suggestions on github, but they're small. This is looking good. Thank you!
Attachment #722260 - Flags: review?(dflanagan) → review-
Attachment #722260 - Flags: review- → review?(dflanagan)
David, I have changed the PR accordingly.
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

Jan,

You're probably getting tired of this bug!  I was just about to give r+ when I noticed an important problem. In the function that shows the dialog, if either callback throws an exception the dialog will never disappear and the app will be blocked.

So please hide the dialog before calling the callbacks.
Attachment #722260 - Flags: review?(dflanagan) → review-
Attachment #722260 - Flags: review- → review?(dflanagan)
Yes, I am ;-) anyway, updated the PR.
Comment on attachment 722260 [details] [diff] [review]
Replace by HTML/CSS confirm dialog

r+, but please add a call to stopPropagation() in the close method.  If you need me to land this, let me know by email, irc or with a needinfo flag.
Attachment #722260 - Flags: review?(dflanagan) → review+
David, please land :-)
Flags: needinfo?(dflanagan)

Comment 20

5 years ago
https://github.com/mozilla-b2g/gaia/commit/81d9e846f1fe34a00db6f2d272ee58eaa4d95cb4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(dflanagan)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.