Convert alert() to ui.dialog [#505598]

Comment	File	Size	Author
#9	alert-to-ui.dialog-1.patch	6.08 KB	tic2000

#3	ui_dialog_stark.png	78.86 KB	kika
	alert-to-ui.dialog.patch	3.99 KB	tic2000

Comment #1

yched CreditAttribution: yched commented 29 June 2009 at 22:31

Would be nice to convert the two other alert() in ahah.js and autocomplete.js ?

Log in or register to post comments

Comment #2

kika CreditAttribution: kika commented 30 June 2009 at 05:14

Status:

Needs review

» Needs work

I propose to add minimal dialog styling (white background + black border?) to our system css so it would also work for Stark. Also, likely Garland jQuery UI CSS should be discussed in a separate issue. So, break it into 2:

- a patch for add minimal UI dialog styling to system css (or a new system-ui.css file?)
- this patch only to convert all alert() -> dialog

Log in or register to post comments

Comment #3

kika CreditAttribution: kika commented 30 June 2009 at 05:15

File	Size
ui_dialog_stark.png	78.86 KB

This is how it looks now in Stark

Log in or register to post comments

Comment #4

kika CreditAttribution: kika commented 30 June 2009 at 05:30

I am not familiar to jQuery UI API, but perhaps "Ok" in this line

+    $('#blocks-dialog').dialog({autoOpen: false, buttons: {"Ok": function() {$(this).dialog("close");}}});

warrants a Drupal.t() ?

But in general, this patch looks more of a proof of a concept than generic reusable component.

I'd like to see a generic theme wrapper theme('dialog', 'error', t('The block cannot be placed in this region.')) or similar

Log in or register to post comments

Comment #5

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented 30 June 2009 at 06:32

In the original design of Garland, I did designed popups to use with drupal. Once things are settled and steady (when we know what could be styled, and what not) I'll look into styling the jQuery UI with a Garlandish touch.

Log in or register to post comments

Comment #6

tic2000 CreditAttribution: tic2000 commented 30 June 2009 at 07:41

@yched
I didn't know there were others. I'll convert them.

@kika
The ui.dialog can be draggable, resizable, can have buttons with custom functions attached to them or none at all, the content can be full html (including forms for example). So when you say you want a theme for it you refer just to the case of replacing the javascript alert, or to a full working one? If it's the former, I can do that, if it's the later, I can do that too, but not before more people agree it's needed.
Also note that even if we do a theme function, that will only take care of adding the correct .js and .css files to the page and the div ui.dialog.js will use and maybe the js code to create the dialog, but you would still have to decide in your js when you want the dialog to trigger. (I hope I made myself clear in what I try to say)

@Stefan Nagtegaal
I just did the little style on Garland so it doesn't look like it does on Stark. I let the design and style decision to others since I'm really not good at designing (I can convert an image to xhtml/css, but I can't make a nice image to start with).

Log in or register to post comments

Comment #7

kika CreditAttribution: kika commented 30 June 2009 at 08:17

@tic2000 Forget about the theme function. I do not yet know how to dialog rendering abstraction should work in detail but what I am proposing is that module devs should be able to simply call "display error alert message with 'blah' in the body". Dialog properties (not resizable etc), title "Error", error icon, red-ish color treatment and (ok) button -- all that should be handled by dialog rendering abstraction.

Also, we might look again at drupal_set_message() and perhaps bring it to wider flexible use: for both "sticky" on-page messages and popup messages.

Log in or register to post comments

Comment #8

tic2000 CreditAttribution: tic2000 commented 30 June 2009 at 09:06

Since at theming level you just need a div with a given ID, I think I'll just make a function (alert or drupal_alert) that expects the id of the div, the title of the alert (it can be something else than "Error") and the text of the error.
If down the line the decision is we want more, we can always develop more functionality.

Log in or register to post comments

Comment #9

tic2000 CreditAttribution: tic2000 commented 30 June 2009 at 13:47

Status:

Needs work

» Needs review

File	Size
alert-to-ui.dialog-1.patch	6.08 KB

New patch. This one does the following:

1. Creates a drupal_add_dialog function that expects the id, title and text.
2. Adds a theme_dialog too (not absolutely needed IMO, but it's there in any case).
3. Add the ui.theme.css also so I don't need to change the Garland style.css file and all that can be done in a separate issue with no need to undo anything. It also now looks "better" in Stark too.
3. Some cleaning while I was at it: you don't need to add tabledrag.js in the template file because drupal_add_tabledrag does that for you; @see theme_block_admin_display() was pointing to a non existing function.

TODO:
I really want to be able to make the inline code work, so it's a more complete solution. Otherwise like in the block case, you need to initialize the dialog.
To convert the other 2 alerts @yched mentioned.

It's still CNW, but I set it to review to see what the bot says and more important, what other people say.

Log in or register to post comments

Comment #10

tic2000 CreditAttribution: tic2000 commented 1 July 2009 at 08:11

Issue tags:

+JavaScript

adding tag

Log in or register to post comments

Comment #11

RobLoach

he/him

CreditAttribution: RobLoach commented 1 July 2009 at 16:42

#315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) might help because then you wouldn't need:

  if (!$js_added) {
    // Add the dialog JavaScript and dependencies to the page before
    // the module JavaScript to ensure that behaviors are registered
    // before any module uses it.
    drupal_add_js('misc/ui/ui.core.js',  array('weight' => JS_DEFAULT - 1.3));
    drupal_add_js('misc/ui/ui.draggable.js', array('weight' => JS_DEFAULT - 1.2));
    drupal_add_js('misc/ui/ui.dialog.js', array('weight' => JS_DEFAULT - 1.1));
    $js_added = TRUE;
  }
  // @todo Find why adding the below code inline doesn't work
  //$js = '(function($){$("#' . $id . '").dialog({autoOpen: true, draggable: true, buttons: {"' . t("OK") . '": function() {$(this).dialog("close");}}});})(jQuery);';
  //drupal_add_js($js, array('type' => 'inline', 'weight' => JS_DEFAULT - 1));
  drupal_add_css('misc/ui/ui.core.css');
  drupal_add_css('misc/ui/ui.draggable.css');
  drupal_add_css('misc/ui/ui.dialog.css');
  drupal_add_css('misc/ui/ui.theme.css');

It would just end up being drupal_add_library('system', 'ui.dialog'). Stefan Nagtegaal, a Garland jQuery UI theme would be great, would you mind making an issue for it?