Hi. First thing first, I wanna say thanks for this module. Really helpful. But there are things that can be improved.
Since the language of my site isn't English, I wanted to translate that modal pop-up interface and at the same time theme the date format and then I realized, that it's impossible without some changes in code. So I made a patch to 'autosave.js' and 'autosave.module'.
About 'autosave.js': nothing really special, I've just added output of 'should-be-translatable' strings through 'Drupal.t("")' function, and thus have changed some if-statements. Check out the patch below.
And what about 'autosave.module', I've added an option to choose in 'Autosave settings' the date formatter (from defined in 'admin/settings/date-time/formats') if module 'Date' is enabled, but I have not entirely succeeded in correct outputting of the date format defined by chosen in 'Autosave settings' formatter — after it was set, formatter returns a machine name of it, and then using date_formatter_format() function I'm getting the actual date format, like (Y.m.d). But it actually returns only year, month and day, and when format additionally has time settings, date_formatter_format() just ignores it and outputting only year, month and day values. Anybody knows the solution to fix that or better way to get date format? See the patch below.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | autosave-translate-1.patch | 3.36 KB | c960657 |
| #2 | autosave.module.patch | 2.56 KB | STINGER_LP |
| #1 | autosave.js_.patch | 3.2 KB | STINGER_LP |
Comments
Comment #1
STINGER_LP commentedComment #2
STINGER_LP commentedComment #3
liquidcms commentedhey, yea t() support is certainly something that should have been added to this ages ago. thanks for submitting the patch.
for the date function, since it is simply a string passed to the JS the simplest way to do this is likely just as a theme function around the date.
so rather than:
something like:
and then of course the theme hook to declare a theme function and the default theme function.
OR there is already a theme_date() function.
this could possibly be extended to include a larger part of the popup; but likely wouldn't bother for the D6 version.
i am sort of torn on making this an admin setting as the D6 version is pretty old now and no one has once asked to modify this; but, if you really think theming isn't the way to go, then you would likely need to have simply a setting for custom date format, and then use the date module's date_format_date() command (which i think has some odd end of month bugs; but might be ok here). But then we would need to add a dependency on the date module.
Comment #4
liquidcms commentedas Crell took over AS for D7 thought i would take a quick look at his code; he does have the t() wrapper in the JS code but also does not handle the date formatting.
Comment #5
liquidcms commentedhey.. sorry hadn't gotten around to reviewing this, but comments:
- i will commit changes for the t() function but not the date options
- if you can create a patch that adheres to Drupal coding stds (and one that works, autosave_extend.js should not be in line 2) i will commit
Comment #6
STINGER_LP commentedOK, thanks. If I'l come up with something, I'll post it.
Comment #7
c960657 commentedI tested the translation patch (autosave.js_.patch) against 2.10, and I can confirm that it works.
This is an updated version (without the date options) that (hopefully) adheres to Drupal coding standards (space aroung the
+operator).Comment #8
liquidcms commentedbetter title
Comment #9
quicksketchThanks, this is a great patch. I'll review it later today for commit.
Comment #10
quicksketchThis patch looks and works great. I reviewed it and everything works as advertised. I'm helping Crell update the D7 version of this module and I thought this problem applied to D7 also, but looks like the JS was largely rewritten for the D7 port and none of these problems apply to D7. Since liquidcms is handling the D6 version, I'll leave this for him to commit but it all looks good to me.
Comment #11
quicksketchWell, heck I'm committing a bunch of backported patches to D6 for consistency anyway, so I might as well include this one too. It's a solid patch and doesn't introduce any problems. Committed to the 6.x-2.x branch.