Allow the user to upload an image for both the bookmark and unbookmark link, to show an image link instead of a text link. Includes previews on the administration page. The only problem is that you have to do a hard refresh to see the new previews - that's not a critical problem though.
The patch is sponsored by Chad Chandler and will be posted in a follow-up (I have to port it to 5.x-1.x-dev).
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | imagelinks_and_effects2.patch | 25.73 KB | wim leers |
| #3 | imagelinks_and_effects.patch | 25.51 KB | wim leers |
Comments
Comment #1
wim leersForgot to mention: this also includes the ability to set effects that will be applied onto the link itself when marking/unmarking from the admin settings.
Comment #2
wim leersThose effects will of course be applied onto the content to which the link belongs, not on the link itself. Sorry for the confusion.
Comment #3
wim leersWell, in the end, I decided to apply the effect on both the content and the link.
Patch attached.
Note to module maintainer: the tabs in the .js file should be removed! My editor fixed some automatically, but I left most of it as it is, to not further complicate this already extensive patch.
Comment #4
wim leersModule maintainer, where are you? Please let me know at least *something* :)
P.S.: another request for this very feature: http://drupal.org/node/166177.
Comment #5
quicksketchHi Wim! Sorry for the really long reply to this issue. With things calming down a bit, I'm cleaning out the queues and have some feedback on this patch.
First, this patch will need a re-roll as it doesn't apply any more as other fixes have been going in. I think this would be a great feature though (after some back and forth decisions about it), and if you can re-roll that'd be awesome.
Other concerns:
- Looks like a lot of tabs slipped into the javascript changes. Try to use 2 spaces everywhere, even in js files.
- Try to use variable tests such as isset() or empty() rather than is_string() to check a NULL value.
Thanks!
Comment #6
quicksketchComment #7
wim leersI'll reroll ASAP, I'm glad this is going in :)
P.S.: I'd swear the tabs were in the .js file already... but it seems they weren't. I'm puzzled how they got in there then, since I recall removing a lot of tabs myself manually :S Anyway, I'll get that fixed, of course.
Comment #8
amitaibusubscribe
Comment #9
momper commentedsubscribe
Comment #10
wim leersRerolled.
- Spaces instead of tabs is fixed. You also had some tabs in your code, this is fixed too.
- isset() instead of is_string(). I have *no clue* why I used is_string(), lol
Comment #11
quicksketchThings look good in general. The PHP code is clean and the update ran without a hitch. Something things that need working though:
- The validation on available effects isn't necessary that I can see. If the user somehow changed the effect with Firebug to something not in the select list, the FAPI prevents them from submitting invalid options already.
- The validate of image types doesn't seem to work. If I try to upload a text file it just says my settings are saved, though no file is ever saved.
- The selector used in the call to applyEffect in the javascript is confounding:
$('div.content + div[a.links_bookmark_'+ vbid +']').prev(). I'm now quite sure what element was being targeted, but on my /node page with several views bookmarks on the page, if the Mark effect is set to Fadeout and the second Mark link effect is set to none, it causes the content area on every node on the page to fade out. Then, more absurd, it flashes the Mark message at me as many times as there are nodes on the page. I tried passing in just $(new_link) instead, which was much more expected but not quite right either. I'm not sure what's trying to be accomplished.- Related to the above. What does the Mark/Unmark link effect actually do? I'm certain it's not working the way it's supposed to, but some help text would be useful below the select list.
- Another thing related to the link effect, is there a reason for really having 2 fields? Could we just have one field that contains 'none' and 'fade' (and possibly 'slide')? Having to configure them separately doesn't seem like it gains us much.
- It looks like one of the image upload forms was wrapped in clear-block, but the Mark upload form was not, causing the Mark link effect (should this be Unmark link effect?) to appear sucked up into the image upload form.
I think the form has become a little daunting with all the new choices, we might consider splitting mark and unmark options into 2 new fieldsets (one for each) to make the form a little more readable. The token replacements help (if token module installed) will need to be put in 2 places now anyway, it doesn't make any sense where it currently is below the image upload form.
Comment #12
chadchandler commentedSubscribed
Comment #13
wim leersI will get this done this weekend probably.
I simply applied my old patch and then started fixing the problems and added the chunks that didn't apply anymore on the go, hence some unlogical stuff.
Thanks for the review!
Comment #14
quicksketchClosing the Views Bookmark queue. This module has been replaced by Flag. If interested in migrating the issue to Flag, please move this issue or open a new issue in the Flag queue.