Popups: Adding the core modal dialog API
starbow - February 7, 2008 - 08:52
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | javascript |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
This patch supports http://drupal.org/node/193311 by providing the basic API for creating, managing and removing JavaScript based, in-page, popup modal dialogs.
It is quite likely that the implementation will change dramatically before Drupal 7 is released (for example, if jQuery UI matures to the point where it makes sense to include it), but, ideally we can settle on a core API and keep that consistent.
#1
Including the animated gif in the patch file seemed weird, so please put the attached ajax-loader.gif into your misc folder.
#2
subscribing
#3
Why does the popup api have it's own javascript file, but not a .css file? That makes no sense ;) Go for consistency.
#4
I am being consistent with all the other core javascript files, which define their css rules in system.css.
#5
Looks like this patch is getting folded back into #193311: Ajax Popups in Drupal 7: Adding Modal Dialogs to Help, Confirmations and Filter tips (Unified). See explination at:
http://drupal.org/node/193311#comment-770004
#6
Here is a first, quick review of the patch. I haven't really looked at the design/approach yet -- I've merely skimmed the code for obvious offenders. I found a couple but nothing too bad:
- The words popup and popups are used inconsistently. Something it is singular, sometimes it is plural.
- The popup settings are way too technical. My mom doesn't know what a 'jQuery selector' is. Can't we remove these settings?
- I'd rename popupsapi.js to popups.js (or popup.js).
- We only use a capital letter for the first word in a title or string. For example, 'Title Selector' should be 'Title selector'. Dito for 'Warning: Please Confirm', 'Save Changes', etc.
- In documentation, we write URL and not url.
- There are various coding style issues: lack of spaces in certain expressions. For example:
'attributes'=>array('class'=>'popup').I'll try to do a more in-depth review shortly -- would be great to get a review from Nate as well.
#7
Thanks for the feedback.
The jQuery selectors are there because there is not currently any other way to find the page's content area. The ideal way to deal with this is to require all themes to use the the ids specified in the default page template (system/page.tpl.php). Then the content area is always #content-content, and the page title is always #page-title.
I touched on this issue for d6 over here: #215312: Missing an id for content div in garland page.tpl.php
A slightly less pushy solution would be to allow the theme creator to use their own content id, but require them to specify it in the theme.info file.
I have cleaned up the naming and capitalization inconsistencies and posted a new patch over at: http://drupal.org/node/193311#comment-773116
#8
Patch still applied.
#9
Based on #6 this needs work.
#10
Yeah, unfortunately it is still stuck in the great JSON logjam. Not to mention the Drupal 6 Popups API module has progressed way beyond this. I would still love to get this functionality into D7, but I have no idea how to move forward.
#11
Subscribing.
#12
It looks like what is proposed in this thread is now implemented in this patch under consideration: #87994: Filter tips modal popup => full jQuery UI integration
#13
stick a fork in it.