jQuery Dirty Forms [#1503176]

This module implements the jQuery Dirty Forms library (http://mal.co.nz/code/jquery-dirty-forms/) which provides a more robust and flexible approach to alerting users when they attempt to leave a page with unsaved changes.

The main benefit of this module over other dirty form libraries is that this one allows you to define custom dialog boxes in newer browsers with a fallback to the browser's default dialog box on older browsers.

Drupal version: Drupal 7
Sandbox project: http://drupal.org/sandbox/Shawn_Smiley/1495354
Git clone: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Shawn_Smiley/1495354.git jquery_dirty_forms

Projects that I'm already a maintainer on:

Views Media Browser (http://drupal.org/project/views_media_browser)
Media Translation (http://drupal.org/project/media_translation)
Media Update (http://drupal.org/project/media_update)
Template Field (http://drupal.org/project/template_field)
Apache Solr Media (http://drupal.org/project/apachesolr_media)

Comment	File	Size	Author
#8	dirty-form.jpg	24.07 KB	vj
	Screen shot 2012-03-26 at 11.51.50 AM.png	61.38 KB	shawn_smiley

Comments

Comment #1

adammalone

English

San Francisco

commented 27 March 2012 at 02:21

Status:

Needs review

» Needs work

I've just taken an automated review for your module and the results can be found here: http://ventral.org/pareview/httpgitdrupalorgsandboxshawnsmiley1495354git

It is likely you'll be able to fix it relatively easily as the majority of issues are coding standards ones such as:

Lines must be indented correctly with 2 spaces instead of tabs.
Files must end on a new line character so it's likely you have an extra line at the end of your jquery_dirty_forms.info

Once those are fixed and your changes are pushed, set the status of the review to 'needs review'.

Comment #2

nick_schuch commented 27 March 2012 at 03:18

I went through a basic install/usability procedure.

1) Installed the module and its required jQuery dependancies.
Might be worth spending some time to create a jquery_dirty_forms.make that can download these libraries.
2) Enabled the module and found the config form under User Interface.
3) Updated textfields for continue and cancel.
4) Updated jQuery Dirty Forms to run on all forms.
5) Edited a form and could only get the message to occur when clicking the breadcrumb links to take me to other pages.

Any chance of this being enabled for the Overlay X (exit) button?
Also I did not receive this message after selecting my browsers back button. (Gmail type functionality).

Upon quick inspection of the code I found (I have not done syntax checking as typhonius has already commented in regards to this):

A large portion of your Hook_Menu implementation was commented out. Is this still required?

Comment #3

shawn_smiley commented 29 March 2012 at 17:04

Thanks typhonius and nick_schuch for your detailed review and feedback.

I've made all of the recommended adjustments including moving the commented out code into a new feature branch (it's for an additional feature I plan on implementing).

This review also revealed a bug in the routines someplace where the dirty forms alerts don't work when the admin overlay is enabled. I'm currently investigating if this is an issue with the external library of the way I'm enabling the library on the site.

Comment #4

shawn_smiley commented 13 April 2012 at 19:07

Status:

Needs work

» Needs review

I've updated the module to fix all coding standard issues and resolve the issue with the alerts not working with the admin overlay.

NOTE: Short term you'll need to use a fork of the jquery.dirtyforms library (see readme file) until my pull request gets merged into the main library.

Comment #5

amauric commented 14 April 2012 at 10:27

Hi,

In the README, line does not exceeds 80 characters

See : http://ventral.org/pareview/httpgitdrupalorgsandboxshawnsmiley1495354git

~~You must add the translation function "t()" in the description of hook_menu~~, Sorry, this is not helpful
In "/js/jquery_dirty_forms.js" there are two blank lines at the beginning of the file,

Comment #6

shawn_smiley commented 15 April 2012 at 22:22

The line length issue has been resolved and I've added in the missing documentation comment at the top of the js file.

Comment #7

vj commented 1 May 2012 at 17:14

Hii,

I have installed this module on my site but as #2 disabled, admin overlay but still i dont get any popups.

Previously i tried my own module as my requirement is only for one content type but that also not succeed.

I am missing something please guide me in right direction.

Comment #8

vj commented 2 May 2012 at 08:51

Status	File	Size
new	dirty-form.jpg	24.07 KB

Its populating the div for popup but not showing in popup.

See the attachment for better understanding.

Is this a bug or i missed something ?

Comment #9

vj commented 2 May 2012 at 09:23

got the answer need to copy all js and css from facebox to src.

Comment #10

shawn_smiley commented 2 May 2012 at 15:48

Thanks Vj for testing things out and letting me know where you ran into problems. Sounds like I need to make some improvements to the install docs and/or process.

Did you get everything working?

Comment #11

vj commented 2 May 2012 at 17:03

yes its working fine for me but actually i want the same functionality for drupal 6 :(

if you have any suggestions please do share

Comment #12

shawn_smiley commented 4 May 2012 at 18:46

This module should be fairly easy to port to Drupal 6 though it's not something I can get to for awhile. I'd be happy to create a 6.x branch if anyone wants to submit a patch.

Comment #13

targoo commented 8 May 2012 at 09:08

variable
I don't know if it is a good idea to define so many variables. It's getting confusing when you need to understand your code.

path
Why is it /sites/all/libraries/jquerydirtyforms/lib/facebox/src/facebox.js and not /sites/all/libraries/facebox/src/facebox.js ?

Use '#attached'
Look into using the new Form API '#attached' property (introduced in D7) to attach Javascript or DSS to a form, instead of using drupal_add_js() or drupal_add_css().

Best,

Comment #14

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 13 June 2012 at 04:45

Priority:	Normal	» Critical
Status:	Needs review	» Needs work

Comment #15

shawn_smiley commented 21 June 2012 at 14:40

Assigned:

Unassigned

» shawn_smiley

Thanks for the review and comments targoo.

To provide some info about your questions/comments:

Why so many variables
Here I'm assuming that you're referring to the number of define() statements that I'm using. I'm a big believer in avoiding duplication. Since all of those values are used in multiple places in the code, it made sense to abstract them out into constants so that the values can be easily changed later if needed.

path to facebox.js
Unfortunately this is a requirement of the jQuery.DirtyForms library which I'm not a maintainer of. The library has hard coded paths to look for the facebox.js library in that specific location. Though I see that the library has undergone some recent updates so I'll revisit that requirement. I would definitely prefer to have facebox as it's own separate library.

Use #attached
My original implementation did this, but I found that didn't work correctly for forms displayed in the admin overlay due to their being inside of an iframe. Thus I needed to load the js as part of the parent page in order to trap cases of the user trying to close the overlay.

Status update on this module
I've been out on vacation for the last month so I haven't had a chance to perform any further updates. Though I just checked and found that the jquery.dirtyforms library on GitHub has been updated again and that the new updated version no longer works with this module. So I need to spend some time tracking down and fixing the integration.