Filter tips modal popup / jQuery UI modal popup helper function
piersonr - October 7, 2006 - 21:40
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | filter.module |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API clean-up, D7DX, DrupalWTF, DX (Developer Experience), jQuery UI, modal dialog, Needs usabiltiy review, Usability |
Description
This function, in theme_filter_tips_more_info, creates a link to /filter/tips within the same window. With some browsers, if you were typing a comment and clicked there in the middle of your comment, the browser tips window would open up in the same window, and then when you hit the back button your message is gone.
Instead this should bring up a pop up window or some sort of javascript hovering trickery.
#1
modified issue version - this is also in the cvs version of the filter module.
#2
#3
#4
This is really a combination of a bug report and feature request, I think.
There is more discussion of the data loss bug at #153313: data loss when navigating away from the "create content" page. So I'm tempted to leave this one open just as the feature request... a popup or modal dialog or something seems like it would be more user-friendly.
#5
One solution to the issue is available at #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations
#6
Marked #394398: Using the help link is clearing previously entered data and #423734: More information about text formats loses text as duplicates of this issue.
#7
The new help system might be used for this, since filter tips are help and the new system includes popup windows.
#13
#15
For now, how about we just do the quick fix and open the link in a new window as this issue's title requests.
#16
It seems this same approach was proposed a long time ago at #59430: "More information about formatting options" must open in new window and marked "won't fix". Worth at least looking at the arguments there...
Not that Drupal is completely strict about this though. I checked, and currently install.php is already using a link with target=_blank.
#17
I am tempted to mark this RTBC. Can we get another code review? And I think most arguments are pretty straight forward, but we simply have to fix this.
#18
I'm opposed to using target=_blank as a "quick fix." You'll certainly have some folks screaming about not validating strict. I would prefer a js/window.open fix. I think we need some alternatives.
#19
Spoke to webchick, basicly this solution is a no go! Sorry :(
#20
Sorry, but as much as I really want this stupid bug fixed, we simply can't fix this with a one-off target="_blank" hack. I believe there was a means of displaying inline help in popups as part of the #299050: Help System - Master Patch patch. Possibly pulling that out and using it here would work.
#21
ok, changed title
#22
I started a drupal_dialog function in common.inc to use jQuery UI's dialog js and css and have this working for filter tips.
#23
The last submitted patch failed testing.
#24
This problem has come up many times in training sessions and also in formal usability testing, IIRC. I'd assume that everyone is +1 to fixing with a modal. With jQuery UI in core, I'd also assume that we'd use its modal dialog library. Jody Lynn, I think your patch correctly moves in this direction.
Where things get tricky is the fact that multiple areas of core could be improved through the use of modal dialogs. What standards are to be used? Reviewing the jQuery UI demo page, you can see that there are many options to be set. For this use case, it could be helpful to allow the dialog box to be moved and re-sized and moved so that one could refer to it during content creation. If modals are used for confirmation messages (ex: node deletion), we'd want to grey out the rest of the screen and disallow resizing.
It would be helpful to have a comment from the D7UX team, Gabor or Webchick regarding the direction for implementing modal dialogs. Should there be a master patch that implements changes in multiple areas or should all the patches be separate? If patches are to be separate, should a usage guideline be created? Should there be a separate "enabler" patch that focuses on providing the helper function (drupal_dialog etc.) before individual implementations are made?
Here are a few related links/issues for background:
- http://hojtsy.hu/blog/2009-jun-17/two-alternatives-d7ux-overlay-implemen...
- #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations
- #193311: Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified)
- #218820: Popups: Adding the core modal dialog API
#25
Yeah. it seems for a minimum drupal_dialog will need to accept settings for the dialog. I'll look more into the past issues as well.
Updated patch fixes the test in php module which failed due to the additional text displayed by the modal and cleans up filter_tips_long function which had no need for the arg() nonsense it was using.
Screenshot attached of the generic jquery ui dialog in effect from this patch.
#26
Mmm... I like the screenshot of the modal dialog. Would this pattern be reusable elsewhere? From the patch code it looks like it would be, but I haven't actually tested the patch.
Modal dialogs might also be useful in some of the areas where people are considering #492834: Hover operations for flooded state screens, especially admin/content/node (see Dries' video in #30 of that issue).
#27
Yeah, it adds a reusable drupal_dialog function you can call anywhere. It won't work in all cases because it uses static text for the modal, no AJAX.
#28
The last submitted patch failed testing.
#29
So this is now the official patch to add modal dialog support to drupal 7?
#30
Subscribe to see where this goes. A few notes on the patch so far:
- We can now get rid of the extensive drupal_add_js/css calls now and just say drupal_add_library('system', 'dialog'); which should add all the JS/CSS files needed.
- Rather than having a set of parameters such as $title and $content, I'd like to see this all just become $options instead, which map directly to the the options provided by jQuery UI's Dialog options. That way we'd have a much more generic use of this function.
- I think keeping $link_class would be a good thing, but it shouldn't be called "link_class". Maybe just something like "class", since it doesn't have to be a link that gets clicked. It's tempting to make this a full selector such as ".link-class" as the value rather than "link-class". We're getting so deep into jQuery I'm not sure CSS/jQuery selectors should scare people any more.
#31
I'm having trouble with drupal_add_library because it seems to be ignoring all but the first usage (I want to use dialogs, draggable and droppable)
I started moving things around so now you specify two jquery selectors (the activator and the modal content selector). I need to implement the options properly still.
#32
Ok this is working well now. You can define a new modal dialog by specifying the selector to be clicked and the selector that has the content, and an array of options.
#33
The last submitted patch failed testing.
#34
I forgot to 'fakeadd' the new file.
#35
Code looks good to me.
This page should really use a render() array but thats probably too much scope creep for this issue. We can do that after freeze.
#36
The results are great, indeed, a very useful and valuable addition. However, the main concern I have now is about the hardcoded dialog dimensions.
How the dialogs dimensions/settings can be altered. Let's say for example, how a module/theme can alter settings of dialogs.
In this case, the dialog is being added inside a theme function, but in other cases, it might be added in menu callbacks, or other places, not necessarily inside theme functions.
#37
Ok, I added a drupal_alter into the drupal_dialog function so you can alter dialogs.
Summary of this patch:
Adds a drupal_dialog function to allow creation of modal windows with settings.
Adds a corresponding misc/dialog.js that uses jQuery UI to implement the dialogs.
Changes the 'more filter tips' link to use drupal_dialog, which creates a draggable resizable modal window.
Context creep in this patch:
Fixes filter_tips_long function which had some unused cruft.
Fixes some unrelated simpletests which superficially break from this patch, due their testing for whether the word 'print' was on the page or not.
#38
Forgot the fakeadd. again.
#39
This patch is clean and does what it says.
I think it's RTBC, anyone care to mark it as such?
+1!
#40
Nice dialog boxes! This does what it says and code looks good.
#41
Soooo... I marked it as such. My bad, +1 anyone?
#42
The big question is: how does this work with the overlay patch?
#43
+++ includes/common.inc 30 Aug 2009 23:48:07 -0000@@ -5188,3 +5188,34 @@ function xmlrpc($url) {
+ * @param $trigger_selector
+ * A jquery selector which should open a dialog when clicked.
+ * Example: 'a#modal-link'
+ * @param $content_selector
+ * A jquery selector which contains the content for display in the dialog.
+ * @param $options
+ * An array of dialog options keyed by option name.
+ * See http://docs.jquery.com/UI/Dialog#options for available options.
Can you combine all this parameters into the $options parameter, and pass 'trigger_selector' and 'content_selector' in the array.
+++ includes/common.inc 30 Aug 2009 23:48:07 -0000@@ -5188,3 +5188,34 @@ function xmlrpc($url) {
+function drupal_dialog($trigger_selector = '', $content_selector = '', $options = array()) {
We already have drupal_add_js(), drupal_add_library(), drupal_add_css(), drupal_add_html_head(), drupal_add_link(), drupal_add_tabledrag(), ..., etc.
Can you follow this common pattern an rename this function to drupal_add_dialog(), for consistency.
+++ misc/dialog.js 30 Aug 2009 23:48:07 -0000@@ -0,0 +1,30 @@
+ attach: function (context, settings) {
+ for (var key in Drupal.settings.dialogs) {
(minor) Wrong indentation here.
+++ misc/dialog.js 30 Aug 2009 23:48:07 -0000@@ -0,0 +1,30 @@
+ }).addClass('dialog-processed');
+ }
(minor) should go in the next line.
#44
#45
Doesn't comply to coding standards, improper indentation in dialog.js.
#46
I'll test this with the overlay patch and make sure everything's copacetic.
But I think in terms of having both the overlay patch and this general simple modal implementation, that that makes since, because the overlay functionality is so much more complex and specific.
#47
I lean towards not putting every parameter into the $options array- those are specifically jQuery UI options for the modal dialog, and sticking everything all together in one array seems more confusing to me.
Everything else I'll change and study the js coding standards.
#48
I made dropcube's changes and hopefully that addressed cwgordon7's indentation concerns as well.
Still need to confirm this works well with overlay patch applied.
#49
I reviewed this patch and made a couple of minor fixes to the code comments (attached). I think this is RTBC.
I also tried it with the overlay patch, and the answer to the question of how it works is "well enough". I was able to click on the filter tips link from within the overlay and get it to pop up on top of that (at least in Firefox) and look pretty much OK. If there are any details to worry about beyond that, I don't think it's the job of this patch to do that.
#50
This is a nice usability improvement. Yet its integration into the render() model was a bit lacking. I've fixed it up. render elements may now add a dialog by setting #attached_dialogs. This goes on to call drupal_add_dialog() which was not changed at all.
I removed the more-info from a theme function since we don't do customizations like this via theme anymore. One can use the $conf['custom_strings'] feature of settings.php or hook_page_alter() to fiddle with the link.
If this patch fails testing, feel free to commit the prior one which is green right now.
#51
The last submitted patch failed testing.
#52
those 4 exceptions have nothing to do with this patch AFAICT.
#53
I thinks it's a better and more extensible approach integrated with render(). A few comments, though.
-function drupal_process_attached(array $libraries = array(), array $js = array(), array $css = array(), $weight = JS_DEFAULT, $dependency_check = FALSE) {+function drupal_process_attached(array $libraries = array(), array $js = array(), array $css = array(), array $dialogs, $weight = JS_DEFAULT, $dependency_check = FALSE) {
There should be a way to 'attach' other stuff further, now it has the parameters fixed, and is not simple to attach other stuff dynamically.
++ // Dialogs to the page.
+ foreach ($dialogs as $dialog) {
+ call_user_func_array('drupal_add_dialog', $dialog);
+ }
+
The same, IMO, we should not do special case for dialogs, rather integrate it with the code we currently have:
<?phpcall_user_func('drupal_add_' . $type, $data, $options);
?>
+ // Allow modules to alter dialogs.+ drupal_alter('dialog', $dialogs);
If the dialogs are attached to the render() page structure, we do NOT need this pecial hook any more. The structure can be altered with hook-page_alter() or other _alter hooks.
+ '#attached_dialog' => array(array(".filter-tips-modal", "#filter-tips-modal-dialog", array('width' => 600, 'height' => 500))),- Sounds better in plural: '#attached_dialogs'
- All the parameters should be combined in one associative array, for example:
<?php'#attached_dialogs' => array(array(
'trigger_selector' => ".filter-tips-modal",
'content_selector' => "#filter-tips-modal-dialog",
'width' => 600,
'height' => 500,
));
?>
This way, it's easier to read, and easier to alter the complete '#attached_dialogs' structure.
Working on a patch to implement these changes.
#54
The following patch implements the changes commented above.
#55
The last submitted patch failed testing.
#56
Ugh. bot says - failed to apply
#57
I have created a separate issue to deal with the dynamic attaching to render() structures that that would be required here: #565496: Allow dynamic attaching of other types of stuff to render() structures .
#58
@moshe: ugh, I had an outdated copy of HEAD, let's re-roll.
#59
Updated patch against HEAD implementing the changes commented in #53 plus other fixes. Note that this patch includes for now the patch from #565496: Allow dynamic attaching of other types of stuff to render() structures, to allow attaching of stuff dynamically instead of doing a special case for dialogs.
#63
Not cool, someone ban that spammer.
#65
(Please?)
#66
The last submitted patch failed testing.
#67
Updated patch, now using #attached.
#68
Thats some nice rendering ... Please wait for green before commit.
#69
I think this needs a usability review - to make sure it matches the overlay style somewhat. And not create a new style, that will confuse the user.
I am totally for committing this by the way, just want to make sure its done well - so we don't encounter issues down the road. Sadly I don't have a local machine here, but I will review later.
No worries about RTBC, this is a usability bug.
#70
Tagging
#71
Looks like wrong status.
#77
Hmmmmm..... I don't want to block this because of overlays. http://drupal.org/node/517688 -- the issue looks troublesome, and it strikes me as funny to shelf a perfectly good usage of a dialog widget - as well as standard code to get them up in running (the first is it in drupal core?!) jQuery UI is such a slick library, and this particular usage is basically by the book: http://jqueryui.com/demos/dialog/#modal-confirmation (minus dropping an overlay).
The patch applied fine, as did playing with it. I've attached screenshots of how it looks. Will wait for 24 hours before i mess with webchick's mellow by marking RTBC
#79
I think it's ready. If there are minor issues with style matching the overlays (it's using all default jQuery UI style now) that can be sorted out later.
#80
I would like to remind everyone, that "now" is later.
#81
I agree this is RTBC.
Note that in #49 above I tested a previous version of this patch with the overlay and it did not look horrible or create any big explosions or anything. I just tried it again with the latest versions now, and the same is true (although the overlay patch currently sets the z-index of the toolbar to a gigantic value, which causes the filter tips to appear underneath the toolbar, but that's a minor conflict that would be easy to resolve).
I don't see any reason to hold this patch up for another patch which does not seem like it's particularly close to being committed. Minor tweaks can be made if and when necessary.
#84
I guess this can be considered RTBC, but it needs a re-roll to fix a few minor issues.
#85
@sun: can you specify what issues should be fixed so that we can fix them ?
#87
+++ modules/php/php.test 6 Sep 2009 20:23:24 -0000@@ -56,7 +56,7 @@
// Make sure that the PHP code shows up as text.
$this->drupalGet('node/' . $node->nid);
- $this->assertText('print', t('PHP code is displayed.'));
+ $this->assertText('print "SimpleTest PHP was executed!"', t('PHP code is displayed.'));
@@ -66,7 +66,7 @@
// Make sure that the PHP code shows up as text.
- $this->assertNoText('print', t('PHP code isn\'t displayed.'));
+ $this->assertNoText('print "SimpleTest PHP was executed!"', t('PHP code isn\'t displayed.'));
Introduced in #37.
Please remove these changes from this patch, unless they're strictly necessary.
+++ modules/filter/filter.module 6 Sep 2009 20:23:23 -0000@@ -635,10 +632,18 @@
+ 'trigger_selector' => '.filter-tips-modal',
+++ misc/dialog.js 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,31 @@
+ $(dialog.content_selector).dialog({
+ autoOpen: false
Trailing white-space here.
+++ modules/filter/filter.module 6 Sep 2009 20:23:23 -0000@@ -635,10 +632,18 @@
+ $form['format_help'] = array_merge($form['format_help'], filter_tips_more_info());
@@ -736,12 +741,16 @@
+function filter_tips_more_info() {
Can we remove this function and just add those elements to the form?
+++ modules/filter/filter.module 6 Sep 2009 20:23:23 -0000@@ -736,12 +741,16 @@
+ '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter Tips') . '">',
"Filter tips" (lower-case t)
+++ includes/common.inc 6 Sep 2009 20:23:15 -0000@@ -5478,3 +5478,40 @@
+ * - 'trigger_selector'
+ * A jQuery selector which should open a dialog when clicked.
+ * Example: 'a#modal-link'.
+ * - 'content_selector'
+ * A jQuery selector which contains the content for display in the dialog.
+ * - 'options'
+ * An array of dialog options keyed by option name.
+ * See @link http://docs.jquery.com/UI/Dialog#options for available
+ * options.
The key description should follow after a colon + space (: ) right after the key name.
Indentation is correct here, though.
The proper syntax for @link is
@link [url] [title] @endlink
+++ misc/dialog.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,31 @@
+Drupal.behaviors.dialog = {
+ attach: function (context, settings) {
There should be a sanity check in here that $.dialog() is actually available.
+++ misc/dialog.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,31 @@
+ if (!$(key).hasClass('dialog-processed')) {
...
+ .addClass('dialog-processed');
Please use Drupal's new $.once() here.
+++ misc/dialog.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,31 @@
+ // Other default settings are defined by http://docs.jquery.com/UI/Dialog.
Such basics can be removed.
This review is powered by Dreditor.
#88
<?php+ $build['long'] = array(
+ '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter Tips') . '">',
+ '#markup' => filter_tips_long(),
+ '#suffix' => '</div>',
+ );
?>
t('Filter Tips') needs to be check-plained.
#89
I don't see how this is a bug, reclassifying.
#90
Sub
#91
@Damien: uhm, no. As of now, we "trust" user interface string translations. Only user-supplied data needs to be escaped. We have many examples in core, so I hope one arbitrary is sufficient to convince you: http://api.drupal.org/api/function/contact_form_user_profile_form_alter/7
Also, this is a usability problem, not really a feature. Since you lose all your form data when clicking on that filter tips link, it can certainly be considered as a bug. However, let's just treat it as a task. ;)
#92
Incorporated issues from #87.
Taking account for #583400: Allow jQuery UI themes to be swappable.
Screenhots for Garland and Seven attached.
#93
The last submitted patch failed testing.
#94
Forgot dialog.js and those php test changes actually seem to be required (not sure why).
#95
This abstracts dialog.js into jqueryui.js and adds
drupal_add_jqueryui()which allows the registration/binding of jQuery UI elements.<?php
// Present the more information about text formats link.
$form['format_help']['more'] = array(
'#markup' => l(t('More information about text formats'), 'filter/tips', array('attributes' => array('class' => array('filter-tips-modal')))),
);
$form['format_help']['long'] = array(
'#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden" title="' . t('Filter tips') . '">',
'#markup' => filter_tips_long(),
'#suffix' => '</div>',
);
// Create a dialog box for the filter tips.
$form['#attached']['drupal_add_jqueryui'][] = array('dialog', '#filter-tips-modal-dialog', array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
));
// When the user clicks on the more information link, present the dialog box.
$form['#attached']['drupal_add_jqueryui'][] = array('dialog', '#filter-tips-modal-dialog', 'open', 'click', '.filter-tips-modal');
?>
As you can see, we create the dialog box, in the first call, and then register the click event to show it. The good thing about this is that it will work on any bindable event, as well as any jQuery UI tool we have available, which gives us quite a bit of power.
Couple side notes:
#96
The last submitted patch failed testing.
#97
Cleaned up
drupal_add_jqueryui()with some documentation. Also added a$baseparameter, so that we're not restricted to "ui" libraries. We can now also usedrupal_add_jqueryui()to add UI Effects as well. Yay screenshot!#98
Great work on this. From a usability/accessibility point of view I really think a drop shadow would add great separation between the content in the dialog and the content behind it.
However I'm not sure what the state of Drop Shadows is in jQueryUI I heard at one point they were removed because they needed more work. I'll have a look at this if I get some time.
#99
Ok, so first things - first. Lets remove the scrollbar - just expand the browsers scroll bar. Then onto the style, as far as I can see the < li >'s cause a lot of indentation - is there anyway we can fix this? (Perhaps some screenshots of other help text's.
Also, can someone run this in sync with overlay to see how it looks, what we change about it visuals.
#100
One thing that the popup is missing is draggable and resizable. Right now once the dialog is presented, you can't move it out of the way and continue writing. We need draggable and resizable jQuery UI libraries so that the user can move it without loosing their place.
Bojhan at #99:
Do you think modifying the actual filter tips content should be in a different issue? Here we're just sticking it in a popup dialog box. Sun brought up the idea of sticking it in jQuery UI tabs/accordion, which might help.
SeanBannister at #98:
Yeah, unfortunately jQuery UI Drop Shadow is still in planning phase of jQuery UI 1.8. We could look at bringing in jQuery Drop Shadow....
Bojhan at #99:
That is the browser's scroll bar ;-) .
Bojhan at #99:
Good idea!
#103
subscribing.
#104
@sun in #94, That php filter test was testing for whether or not the word 'print' was appearing on the screen (not the most robust test). The filter tips contain the word 'print', so it broke the test.
#105
I updated the filter_tips_long code (looks like there was a recent change to theme_filter_tips that affected it), and added draggable and resizable as dependencies to the dialog js, which makes the dialog draggable and resizable. (Which is in agreement with http://docs.jquery.com/UI/Dialog#overview which lists them as dependencies)
I think the theming recommendations are out of this issue's scope. We haven't done any theming at all to the dialog, it's just jQueryUI out of the box. Theming dialogs or the filter tips content should be new issues.
#106
The last submitted patch failed testing.
#107
Forgot my own advice in #104
#108
The last submitted patch failed testing.
#109
Rerolled Jody's last patch to HEAD without the PHP test update. I really like it with the resizable and draggable.
Here's a Screenshot of it with #583400-19: Allow jQuery UI themes to be swappable :-) .
#110
The last submitted patch failed testing.
#111
Bojhan at #99 asked to see how this works with the Overlay, so I took the steps I outlined at #583400-19: Allow jQuery UI themes to be swappable, and then applied the latest patch in #517688: Initial D7UX admin overlay, and the attached screenshot is the result. Seems to work quite well!
Through #583400: Allow jQuery UI themes to be swappable, we could give Seven's jQuery UI theme a look that more matches the administrative stuff. Theme roller and theme_hook_alter to the rescue! Yay! :-)
Anyone know why the PHP test failed?
#112
The code discussed in #104 and #107 needs to be added back to the patch to make the tests pass again.
#113
Looking great with #583400. Seems like we just need to reroll with that php test adjustment and we're golden.
#114
Hmmm, bizarre.
#115
Jody in #113 says that everything checks out. #111 addresses the way forward and compatibility. All tests pass in #114. Is this RTBC?
#116
@RobLoach: Very nice job!
+++ includes/common.inc 14 Oct 2009 16:57:21 -0000@@ -3673,6 +3673,55 @@
+ * @param $tool
+ * The name of the tool to add (dialog, accordion, resizable, etc).
...
+ * @param $base
+ * (optional) The base name of the tool that is being added. Specify "effect"
+ * for any jQuery UI effect, and "ui" for widgets and interactions.
...
+function drupal_add_jqueryui($tool, $selector = NULL, $options = array(), $event = 'ready', $binded_element = NULL, $base = 'ui') {
...
+ drupal_add_library('system', "$base.$tool");
The order of arguments looks very unnatural to me. I'd expect $base, $tool, and both being required.
+++ includes/common.inc 14 Oct 2009 16:57:21 -0000@@ -3673,6 +3673,55 @@
+ * @param $selector
+ * (optional) The CSS selector of which to apply the tool.
s/of which//
+++ includes/common.inc 14 Oct 2009 16:57:21 -0000@@ -3673,6 +3673,55 @@
+ * @param $event
+ * (optional) On what binded event the tool should be applied to the element.
+ * Defaults to document ready.
"document ready" ?
Perhaps, "document.ready()".
Or, "document's 'ready'".
+++ includes/common.inc 14 Oct 2009 16:57:21 -0000@@ -3673,6 +3673,55 @@
+ * @param $binded_element
+ * (optional) When binding on an event other then ready, will be the element
+ * that the event is binded to.
s/then/than/
+++ misc/jqueryui.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,45 @@
+Drupal.behaviors.jqueryui = {
...
+ if (settings.jqueryui || false) {
...
+ // Apply the jQuery UI's effect.
+ $(selector, context).once('jqueryui-' + tool + '-ready')[tool](options.options);
...
+ // Apply the jQuery UI's effect on a binded event.
+ $(options.item, context).once('jqueryui-' + tool + '-' + event).bind(event, function(e) {
Not 100% sure, but I'd prefer if core would take away the "ui" behavior namespace (i.e. skipping the "jquery" prefix).
So also this file would be renamed to ui.js.
Because, we still don't know about the future of the contributed jquery_ui module...
This review is powered by Dreditor.
#117
Thanks for the review. I've included the notes sun made in this patch.
drupal_add_ui(), along with renamed jqueryui.js to ui.js and the namespace change$baseargument. You're right in this not really being optional.... Got an idea regarding
$base, we could remove the base part from the related jQuery UI libraries in system_library so we're left with$libraries['dialog']instead of$libraries['ui.dialog'], and$libraries['explode']instead of$libraries['ui.explode']. Then we wouldn't need this $base argument at all. Thoughts?#118
After a quick discussion with tha_sun and DamZ, we ended up going with:
<?php
drupal_add_ui($module, $library, $selector, $options, etc)....
// Create a dialog box for the filter tips.
$form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
));
// When the user clicks on the more information link, present the dialog box.
$form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', 'open', 'click', '.filter-tips-modal');
?>
This matches the function definition of drupal_add_library for added bonus consistency.
#119
Forgot to remove the $base PHP docs.
#120
I love it.
This may even be usable for Overlay.
#123
#124
I like this, but I think it is secondary to the main overlay patch at #517688: Initial D7UX admin overlay. I'd like to get the main Overlay in first, and then see how both play together. I think that is the best, and most natural order, that would give us the biggest bang.
#125
The last submitted patch failed testing.
#126
Rerolled. And yup, it works alongside the Overly! :-) http://drupal.org/files/issues/OverlayAndFilterTips.png from #583400-19: Allow jQuery UI themes to be swappable.
#127
The last submitted patch failed testing.
#128
The theme_filter_tips_more_info function was removed, but it was still being used in filter.admin.inc. I fixed that, but we probably want to just put a modal dialog in the filter admin as well.
#129
I added the modal for the filter admin pages (e.g. /admin/config/content/formats/1)
#130
Yup!
Just created a follow up issue: #609094: Popups for the "More Help" links.
#131
The last submitted patch failed testing.
#132
Any update here? We're now a week past freeze #2 and overlay's status is unclear. I would hate to see a usability improvement like this one passed over while waiting for a mega-patch that may or may not land (...and I do hope it lands).
#133
Brought back to HEAD.
#134
Index: modules/system/system.module===================================================================
RCS file: /cvs/drupal/drupal/modules/system/system.module,v
retrieving revision 1.826
diff -u -r1.826 system.module
--- modules/system/system.module 25 Oct 2009 19:52:47 -0000 1.826
+++ modules/system/system.module 26 Oct 2009 04:04:57 -0000
@@ -1153,6 +1153,8 @@
Please roll with -p
+++ modules/system/system.module 26 Oct 2009 04:04:57 -0000@@ -1153,6 +1153,8 @@
Index: modules/php/php.test
Index: modules/php/php.test
===================================================================
===================================================================
RCS file: /cvs/drupal/drupal/modules/php/php.test,v
RCS file: /cvs/drupal/drupal/modules/php/php.test,v
retrieving revision 1.18
retrieving revision 1.18
diff -u -r1.18 php.test
diff -u -r1.18 php.test
--- modules/php/php.test 11 Oct 2009 03:07:19 -0000 1.18
--- modules/php/php.test 11 Oct 2009 03:07:19 -0000 1.18
+++ modules/php/php.test 26 Oct 2009 04:04:57 -0000
+++ modules/php/php.test 26 Oct 2009 04:04:57 -0000
+++ modules/php/php.test 26 Oct 2009 04:04:57 -0000
@@ -60,7 +60,7 @@
do you really need these changes?
+++ includes/common.inc 26 Oct 2009 04:04:57 -0000@@ -3882,6 +3882,58 @@
+ * @param $module
+ * The name of the module that registered the desired library. For most jQuery
+ * UI elements, this is usually "system".
What jQuery UI elements would not be defined by system? none! Please remove $module altogether
+++ includes/common.inc 26 Oct 2009 04:04:57 -0000@@ -3882,6 +3882,58 @@
+ * An array representing all elements added to the page so far.
+ */
+function drupal_add_ui($module, $library, $selector = NULL, $options = array(), $event = 'ready', $binded_element = NULL) {
use a big $options array for all but the first two items please. Also, it's bound, not binded
+++ modules/filter/filter.module 26 Oct 2009 04:04:57 -0000@@ -674,10 +671,28 @@
+
+ // Create a dialog box for the filter tips.
+ $form['#attached']['drupal_add_ui'][] = array('system', 'ui.dialog', '#filter-tips-modal-dialog', array(
+ 'width' => 600,
what about just a #attached[ui] ?
#135
Dmitri: Comment #104 explains why we need that change to php.test
#136
Thanks for giving it a look, Dmitri! I like the options array idea, as well as [#attached][ui].....
<?php// Create a dialog box for the filter tips.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'options' => array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
),
);
// When the user clicks on the more information link, present the dialog box.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'options' => 'open',
'event' => 'click',
'bound_element' => '.filter-tips-modal'
);
?>
#137
Cleaned up the code a bit.
#138
Obviously I agree with Dries that Overlay should go in first, but given the progress on that issue - I think its smart to commit this anyway. Since it is almost always already demonstrated with the Overlay, we know the visual issues are fixable and no bugs have been encounterd yet.
#139
+++ includes/common.inc 27 Oct 2009 17:53:46 -0000@@ -3882,6 +3889,66 @@
+ // Add the settings so that the behaviors are attached to the elements.
+ if (isset($options['selector'])) {
+ // Use the explicit index to avoid conflicts when the settings are merged.
+ $index = count($elements[$library]);
+ // Remove items that the JavaScript doesn't need and add the settings.
+ unset($options['module'], $options['library']);
+ $settings['ui'][$tool][$index] = $options;
+ drupal_add_js($settings, 'setting');
+ }
My mistake. This should add an item to $elements[$library][$tool], and then get the count of $elements[$library][$tool] instead so that $index keeps growing correctly. As it is now, $index will stay on how many tools you add, as oppose to how many jQuery UI elements you invoke. With this, when the settings are merged, you'd hit conflicts.
This review is powered by Dreditor.
#140
Braindump.....
applyJavaScript function.#141
I'm slightly worried about calling the new function
drupal_add_ui(). I'm worried novice developers will think that means "output the form elements" or something. ;) I'd rather it was calleddrupal_add_jqueryui()or something, since it's entirely specific to jQuery UI, right?Also, I wish I knew why we need to do this twice, more or less:
<?php// Create a dialog box for the filter tips.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'options' => array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
),
);
// When the user clicks on the more information link, present the dialog box.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'options' => 'open',
'event' => 'click',
'bound_element' => '.filter-tips-modal'
);
?>
Isn't there some better way to handle this? Can't we merge the definition of the dialog and defining how it's attached to various triggers into the same step? Even if jQuery UI itself can't handle that, and jQuery UI really thinks of this as 2 actions, couldn't we handle that ourselves in drupal_add_jqueryui() to avoid the duplication at every call site? Roughly:
<?php// Create a dialog box for the filter tips.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'definition' => array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
),
'actions' => array(
// When the user clicks on the more information link, present the dialog box.
array(
'options' => 'open',
'event' => 'click',
'bound_element' => '.filter-tips-modal',
),
),
);
?>
Also, is there a good way to associate actions with the "okay" and "cancel" buttons on the dialog? E.g. if I want to trigger something when the user clicks "cancel" in the dialog, how would I do that?
Otherwise, this is looking very cool. I applied the patch and played with the filter help tip popup -- super yummy!
Thanks,
-Derek
#142
This patch adds a couple things:
<?php// Create a dialog box for the filter tips.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'arguments' => array(
array(
'width' => 600,
'height' => 500,
'dialogClass' => 'filter-tips',
'autoOpen' => FALSE,
),
),
);
// When the user clicks on the more information link, present the dialog box.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'arguments' => array('open'),
'event' => 'click',
'bound_element' => '.filter-tips-modal',
);
?>
It would be great to have the ability to call multiple "actions" on a given element like Derek suggested above.
#143
Whoops, wrong patch.
#144
Adds a "functions" associative array, which allows declaration of JavaScript functions for use in the "arguments" array. This is handy when you want to have a function callback passed into, say a jQuery UI Dialog button, like in #613654-11: update.manager.js is missing.
#145
Interesting...... #617730: Usability: Popup node delete confirmation form.
#146
Added in this patch
Things to consider
drupal_add_ui()#147
Awesome!
Not sure what to do about the multiple invocation thing. Basically, each invocation seems to add cruft to the settings array, so it seems to be worth to consider multiple things for the same module + library + selector for bandwidth reasons alone.
+++ includes/common.inc 30 Oct 2009 19:12:33 -0000@@ -3882,6 +3889,88 @@
/**
+ * Adds a jQuery UI element to the page, and invokes its behaviors.
+ *
...
+function drupal_add_ui(array $options = array()) {
We need at least one example usage in the PHPDoc here.
+++ modules/filter/filter.admin.inc 30 Oct 2009 19:12:33 -0000@@ -160,7 +160,30 @@
+ 'library' => 'ui.dialog',
+ 'selector' => '#filter-tips-modal-dialog',
+ 'arguments' => array('open'),
+ 'event' => 'click',
+ 'bound_element' => '.filter-tips-modal',
It seems like the more natural order is:
library
selector
event
bound_element (optional)
arguments
The @param description of drupal_add_ui() should use the very same order, having any other (totally optional) parameters appended last.
+++ modules/filter/filter.module 30 Oct 2009 19:12:34 -0000@@ -674,10 +671,40 @@
+ // Present the more information about text formats link.
+ $form['format_help']['more'] = array(
+ '#markup' => l(t('More information about text formats'), 'filter/tips', array('attributes' => array('class' => array('filter-tips-modal')))),
+ );
+ $form['format_help']['long'] = array(
+ '#prefix' => '<div id="filter-tips-modal-dialog" class="ui-helper-hidden">',
+ '#markup' => filter_tips_long(),
+ '#suffix' => '</div>',
+ );
+
+ // Create a dialog box for the filter tips.
+ $form['#attached']['ui'][] = array(
+ 'library' => 'ui.dialog',
+ 'selector' => '#filter-tips-modal-dialog',
+ 'arguments' => array(
+ array(
+ 'width' => 600,
+ 'height' => 500,
+ 'autoOpen' => FALSE,
+ 'title' => t('Filter tips'),
+ ),
+ ),
+ );
+ // When the user clicks on the more information link, present the dialog box.
+ $form['#attached']['ui'][] = array(
+ 'library' => 'ui.dialog',
+ 'selector' => '#filter-tips-modal-dialog',
+ 'arguments' => array('open'),
+ 'event' => 'click',
+ 'bound_element' => '.filter-tips-modal',
+ );
Looks like we need a helper function now. Makes no sense to have the identical code.
+++ misc/ui.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,89 @@
+/**
+ * @file
+ * Provides the jQuery UI Drupal behaviors.
+ */
+
+ /**
+ * The base UI namespace.
+ */
+ Drupal.ui = {
Strange indentation.
+++ misc/ui.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,89 @@
+ jQuery.each(elements, function(index, value) {
...
+ jQuery.each(settings.ui, function(action, effects) {
+ jQuery.each(effects, function(index, effect) {
...
+ jQuery.each(effect.functions, function(name, code) {
We can use $ here.
+++ misc/ui.js 1 Jan 1970 00:00:00 -0000@@ -0,0 +1,89 @@
+ attach: function(context, settings) {
(and elsewhere) Anonymous functions should have a space between 'function' and the opening parenthesis.
I'm on crack. Are you, too?
#148
I hit some of the tha_sun's comments, as well as added dww's "actions" parameter. I find it helps a bit with the developer experience.....
<?php// Create a dialog box for the filter tips.
$form['#attached']['ui'][] = array(
'library' => 'ui.dialog',
'selector' => '#filter-tips-modal-dialog',
'arguments' => array(
array(
'width' => 600,
'height' => 500,
'autoOpen' => FALSE,
'title' => t('Filter tips'),
),
),
'actions' => array(
// When the more information link is clicked, open the dialog box.
array(
'event' => 'click',
'bound_element' => '.filter-tips-modal',
'arguments' => array('open'),
),
),
);
?>
.... Is it just me or does
$(this)[action].apply($(this), effect.arguments);not work on Safari or Internet Explorer? It works fine in Firefox.