Closed (outdated)
Project:
Hover Preview for ImageCache
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
8 May 2011 at 00:37 UTC
Updated:
11 Oct 2016 at 16:43 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
chx commentedThat was the least of it. This takes the CSS and JS out of 6.x and makes it work. The D7 JS is gone. It was unusable. The formatter settings form were usable so that was kept. The formatter itself is practically rewritten and the theme is totally new even if most of it is copypasted from image.field.inc.
Please credit alippai as well when committing this. Huge props to him for helping with the port.
Comment #2
chx commentedThis one is even better as it fixes the bug where the hover_image_style was not filled in correctly.
Comment #3
chx commentedNote that #1116486: Views field configurations do not save correctly. is also solved by this.
Comment #4
robloachAwesome! Thanks chx. I'll commit this tomorrow..... Mind taking a look at #1025796: Rename path to uri in image functions too? Would be nice to get the image attributes fix into core itself. The tests should pass once that user test passes.
Once the attributes are passed through to the image, we could pass in a "data-hoverpreview" attribute with the hover image.
Comment #5
robloachAssigning to myself so I see it in my critical queue.
Comment #6
chx commentedNot sure what you want with data-hover-preview, that was only present in the old D7 javascript which was broken beyond my abilities to repair. It seemed to me that one was just changing the image source url without offering a nice hover "popup". The patch includes the D6 JS ported to D7 instead ( I wrapped it in the function ($) magic and changed the function to
Drupal.behaviors.HoverPreview = { attach : function (context, setings)but otherwise it's the D6 JS untouched, diff it to see.)Comment #7
chx commentedI got a warning that #2 was not what i wanted. Correct.
Comment #8
chx commentedSupporting none as hover_preview_style.
Comment #9
chx commentedLittle leftover caused linked images to break.
Comment #10
chx commentedD'oh!
Comment #11
robloachI applied the patch and went through, touched up a bunch of it, and committed it. Thanks a lot for the help on this.
Comment #12
chx commentedSo you have decided to throw our work away 100% and commit something completely untested? Thanks.
Comment #13
chx commentedThe questionable decision in core was whether it was a good idea not to send $item to theme image_style / image directly. There is no doubt however that omitting a theme function is an incredible bad idea -- you killed the possibility of preprocess. And, using theme image always and messing around with image_style_url yourself is an incredibly un-Drupalish thing to do. I won't even mention that you are using HTML tags (<a>) in a non-theme function.
What was wrong with the code I submitted? It is as close to battle tested pieces -- namely core and D6 -- as possible.
Comment #14
robloachHi chx... The module is not broken. It works, and it works well. I just committed a fix that adds back the theme formatter. Can't think of a reason why you would ever want a theme override for this as most of the theming is done on the JavaScript end, but it's now back in there. You're right about having the tag, it's now part of the theme function instead.
As for the Drupal 6 JavaScript code in there before, it was gross and ugly, and there are jQuery Plugins around that accomplish what we we were trying to do. That's why I've opted to use imgPreview and imgHover here.
If you have any more concerns, please feel free to let me know. The module does work, and it has more features then the Drupal 6 version. I think I've fixed up most of your gripes here. Dealing with image_style_url instead of passing the $item in manually is a good idea, but I think it should wait until #1025796: Rename path to uri in image functions gets in. We need to pass attributes into the image, and the attributes are ignored if we pass $item directly into theme_image_formatter.
Let me know what you think. Thanks a lot.
Comment #15
chx commentedStill no dice. You are not calling theme('image_style') which you should, twice, why do you think I wrote a helper? But, it's getting closer. If you want these jQuery plugins, I guess that's fine. Just please do not skip the core-established theme functions.
Comment #16
robloachRenaming the title to more closely reflect what the goal here is. Thanks for the help again chx.
Comment #17
naeluh commentedsubscribe
Comment #18
robloachThe module's usage has gone up since this has gone in. Module is working, so not a critical issue. If someone wants to patch this, then good on them :-) .
Comment #19
Andre-BI am cleaning up the issue queue. If this is still an issue for you and relevant. please reopen and attach a patch against 7.x-1.x