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.
should not block on polish
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?
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.)
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.
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.
(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.
Alright, can we merge this?
(In reply to janjongboom from comment #10) > Alright, can we merge this? Looks good to me.
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.
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+.
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!
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.
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.
David, please land :-)