Posted by profix898 on October 9, 2006 at 11:09am
| Project: | quicktags |
| Version: | master |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
Issue Summary
Here is the code to bring Quicktags to Drupal 5.0. Archive contains whole Quicktags module based on latest cvs version.
- images moved to /images/ subfolder for consistency
- deprecated hook_settings replaced with menu callback
- .info file added
- add css using drupal_add_css()
... guess thats it basically ...
I didnt touch the javascript, but of course it can be updated to use jQuery!
| Attachment | Size |
|---|---|
| quicktags5.zip | 13.9 KB |
Comments
#1
Hello,
First of all, please don't post zip files. They make reviews a lot harder. check http://drupal.org/diffandpatch for more info.
Secondly, I will not branch to 5 until we have a way to release 4.7.x releases for contribs. Else we "loose" all the good and hard work gone into the 4.7-compatible HEAD version.
For ther rest, thanks a lot for the hard work. I will comment on the code(style) once i have a patch to look at.
#2
Ok. But AFAIK you cant move files in patches ;)
It was not hard work, but a quick run over the code ...
I was just needing a 5.0 compatible version of quicktags to play with hook_quicktags_insert in one of my own modules to port that one to 5.0. Its a little nasty if a module providing an api/hook (like quicktags) is not available for testing.
Here the patch (and remember I moved the images to /images/ folder) ...
#3
Accidently changed too much, here a corrected patch.
#4
Hi,
I know you cannot move files in patches. But neither can you in a zip file. Hence the best option is to list what files need to:
* be moved
* be removed
* be added (and upload these files too).
And off course patches so I can review the changes.
Review:
* please adhere to Drupal coding standards;
} else { is wrong.
* you move images into /images, but leave the javascript and CSS in place, should we not move them into /styles/ and /javascripts/ too?
* 'description' => 'Configure Quicktags options.', must be t() ed. And mind capitalisation.
Bèr
#5
#6
I didnt move js/css because its only one file each, but there were multiple images (to be grouped into a subfolder). The new patch also moves js/css as you suggested.
List of files to be moved:
- move all images 'ed_*.png' to '/images/' subfolder
- move 'quicktags.js' to '/javascript/' subfolder
- move 'quicktags.css' to '/styles/' subfolder
Your other issues (t() and else) are also corrected.
The patch still includes the 'hook to include javascript' stuff, but if you decide not to commit #88234, I can remove it. So its up to you ;)
#7
And a patch with only Drupal 5 stuff (just to be sure).
#8
Here is an update on my previous patch against HEAD (without moving images,... into seperate folders). It simply brings the latest quicktags.module (1.20) to Drupal 5 (incl. .info file.
#9
I just installed this on my new 5.0 RC1 installation, and it works great! I am so glad to have my Quicktags back.
Thanks Thilo! It's working wonderfully well. Please add this to the list for the compatible 5.0 modules - at least on the head. Some of us regular, non-programmer types have a hard time applying patches. It's much easier to understand a complete archive containing the latest and the greatest that we can simply upload, then activate.
Thanks again,
-Brent
#10
merging issue 98932 into this one. Lets keep our actions synchronised and centralised please.
#11
And here is Digital Spaghetti's patch. Being the last patch, does NOT mean its the best or the "one". My personal preference goes out to the patch in http://drupal.org/node/88200#comment-169510. I need some people to review both patches and possibly merge them into one final one.
#12
http://drupal.org/node/98932 was marked duplicate of this issue.
#13
There's nothing to merge into #8. The patch already contains the .info patch from #10 (without
package = quicktags, this shouldnt be used for single module packages). It also imports the stylesheet correctly (from #11).#14
(I also suggest that we remove the ?q=quicktags code (see http://drupal.org/node/88234), what makes the menu callback redundant and also solves the javascript include issue (without any additional code/hook ;). This is not much more than code cleanup IMO. Modified patch attached.)
#15
Subscribing. The users on the site I'm building really want this feature.
I haven't looked at the contents of any of the patches, but with multiple revisions of the patches I'm fearing I'll get confused trying to work out how to apply it. Is it possible to produce straightforward instructions? Or ??
#16
Take a look at http://drupal.org/node/60108 'Howto: Apply patches'. All the patches are against the latest version of Quicktags. What means you download latest Quicktags or get it from cvs and apply the patch. Then test it and post your opinion: Does it work? Any problems? Does the code look clean? ...
#17
Note that the last part
is also in your own benefit: It means that with positive reviews I can commit the patch , and you can start running the stable version on your site sooner.
#18
I know how to use the patch program .. I've been using it since the 1980's. What I was asking is which of the five or so patches on this issue do I use? And some of the comments talk about moving files around, and I don't see it clearly described what files to move where, etc.
#19
Try the patches in #8 and #14 in the first place. #8 is a simple port of the latest Quicktags (without any files being moved) and #14 additionally makes a small improvment to the code that adds the javascript.
#20
I just applied the patch in comment #14. The test site is running rc1.
All the buttons worked and inserted stuff appropriately. I'm happy with the behavior of each of the buttons.
The <!--break--> when inserted shows on the screen as <!--break--> ... don't you mean that to be the signal to Drupal for a break on the teaser? The node type is blog-entry.
#21
Patch #14 works for me running Drupal 5.0.
I guess the <!--break--> issue has been fixed in 5-RC2 and of course 5.0.
#22
It was not so much broken, and therefore, is not 'fixed' now either. Problem is that core decided to introduce a hardcoded solution for teasers. We must make sure quicktags at least works toghether with that solution.
If that is not possible, then we must develop something to unset the core-behaviour, and if possible, create a solution in quicktags that emulates that core behaviour.
#23
Patch works for me. I think its good enough to commit and move on.
#24
neclimdul: what patch did you test?
#25
To add to the confusion, without knowing about this thread, I've created my own version of quicktags, without the fugly icons.
http://drupal.org/node/113146
#26
whoops, sorry for the accidental rename.
#27
Was reviewing:
http://drupal.org/node/88200#comment-169703
Looking at it again I'd kinda like the menu array to be indented to the coding standards but the code works just fine.
#28
Tested code in #14 and it works fine.
Subscribing so I know when the changes get commited.
#29
I tried to exoplain before, that I am not happy with the fact that this patch introduces new features (i.e. add the JS to the HTML body). Not that that is not a good feature, I like it, but it keeps me from the REAL purpose of this patch, to upgrade to 5.x.
Therefore, I a closing this thread. It is fixed in dedicated threads:
http://drupal.org/node/115580
http://drupal.org/node/115583
http://drupal.org/node/115585
And one issue is still open in thread:
http://drupal.org/node/115586