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.

CommentFileSizeAuthor
#1 popupapi.patch10.24 KBstarbow
#1 ajax-loader.gif8.04 KBstarbow

Comments

starbow’s picture

starbow commented
Assigned: Unassigned » starbow
Status: Active » Needs review
StatusFileSize
new ajax-loader.gif8.04 KB
new popupapi.patch10.24 KB

Including the animated gif in the patch file seemed weird, so please put the attached ajax-loader.gif into your misc folder.

steven jones’s picture

steven jones commented

subscribing

breyten’s picture

breyten commented

Why does the popup api have it's own javascript file, but not a .css file? That makes no sense ;) Go for consistency.

starbow’s picture

starbow commented

I am being consistent with all the other core javascript files, which define their css rules in system.css.

starbow’s picture

starbow commented

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

dries’s picture

dries commented

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.

starbow’s picture

starbow commented

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

lilou’s picture

lilou commented
Category: task » feature

Patch still applied.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Based on #6 this needs work.

starbow’s picture

starbow commented
Assigned: starbow » Unassigned

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.

babbage’s picture

babbage commented

Subscribing.

babbage’s picture

babbage commented

It looks like what is proposed in this thread is now implemented in this patch under consideration: #87994: Quit clobbering people's work when they click the filter tips link

starbow’s picture

starbow commented
Status: Needs work » Closed (fixed)

stick a fork in it.

lourencofernandes’s picture

lourencofernandes commented
Status: Closed (fixed) » Fixed

drupal7 :(

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.