backdrop-contrib / colorbox

A port of the Colorbox Drupal 7 module to BackdropCMS
GNU General Public License v2.0
3 stars 7 forks source link

Undefined Variable library #1

Closed krbennett closed 8 years ago

krbennett commented 9 years ago

This could just be something I have not set up correctly... I added the colorbox module and after accepting the defaults on the configuration I get this: Notice: Undefined variable: library in colorbox_admin_settings() (line 62 of ...modules/colorbox/colorbox.admin.inc). it repeats for lines 63, 64, 65, 66

I have went ahead and set up a content type with images. If image is selected, I see the image. If colorbox is selected no image is visible.

biolithic commented 9 years ago

Sorry, will test and update or patch tomorrow. Thanks for the post On May 10, 2015 8:20 AM, "krbennett" notifications@github.com wrote:

This could just be something I have not set up correctly... I added the colorbox module and after accepting the defaults on the configuration I get this: Notice: Undefined variable: library in colorbox_admin_settings() (line 62 of ...modules/colorbox/colorbox.admin.inc). it repeats for lines 63, 64, 65, 66

I have went ahead and set up a content type with images. If image is selected, I see the image. If colorbox is selected no image is visible.

— Reply to this email directly or view it on GitHub https://github.com/backdrop-contrib/colorbox/issues/1.

biolithic commented 9 years ago

Hi! With the acceptance of libraries module into Backdrop (I thought there was going to be a different solution or it would be axed), I think I will rework this module to work with Libraries instead of how it currently is an all-in-one solution. Any questions? Otherwise, thank you for your patience!

krbennett commented 9 years ago

That sounds like a good idea. I have a lot of small sites, mostly using Drupal 7. I want to move to Backdrop if I can, I really hope Backdrop succeeds. I was wanting to upgrade one of my few remaining Drupal 6 sites using a fresh install of Backdrop. I needed colorbox - I have time to wait. Will be glad to test if that helps.

Thank you,

Karen

-----Original Message----- From: "Andy Martha" notifications@github.com Sent: Monday, May 11, 2015 6:48pm To: "backdrop-contrib/colorbox" colorbox@noreply.github.com Cc: "krbennett" kbennett@1791.com Subject: Re: [colorbox] Undefined Variable library (#1)

Hi! With the acceptance of libraries module into Backdrop (I thought there was going to be a different solution or it would be axed), I think I will rework this module to work with Libraries instead of how it currently is an all-in-one solution. Any questions? Otherwise, thank you for your patience!


Reply to this email directly or view it on GitHub: https://github.com/backdrop-contrib/colorbox/issues/1#issuecomment-101076225

biolithic commented 9 years ago

Hi! Thanks for saying this. I agree that Colorbox is a needed module, and Backdrop 1.1 is coming out this Friday which is worth waiting for. If you are familiar with how Colorbox works with Drupal 7 and Libraries, I will change it back to that

On Mon, May 11, 2015 at 7:04 PM, krbennett notifications@github.com wrote:

That sounds like a good idea. I have a lot of small sites, mostly using Drupal 7. I want to move to Backdrop if I can, I really hope Backdrop succeeds. I was wanting to upgrade one of my few remaining Drupal 6 sites using a fresh install of Backdrop. I needed colorbox - I have time to wait. Will be glad to test if that helps.

Thank you,

Karen

-----Original Message----- From: "Andy Martha" notifications@github.com Sent: Monday, May 11, 2015 6:48pm To: "backdrop-contrib/colorbox" colorbox@noreply.github.com Cc: "krbennett" kbennett@1791.com Subject: Re: [colorbox] Undefined Variable library (#1)

Hi! With the acceptance of libraries module into Backdrop (I thought there was going to be a different solution or it would be axed), I think I will rework this module to work with Libraries instead of how it currently is an all-in-one solution. Any questions? Otherwise, thank you for your patience!


Reply to this email directly or view it on GitHub:

https://github.com/backdrop-contrib/colorbox/issues/1#issuecomment-101076225

— Reply to this email directly or view it on GitHub https://github.com/backdrop-contrib/colorbox/issues/1#issuecomment-101078225 .

Graham-72 commented 9 years ago

Hi @biolithic - I too am wanting to use Colorbox, just as I do in Drupal 7. Thanks.

krbennett commented 9 years ago

I tried the new your new version. Better, but I still have problems. I set the Image style to use in the content to the existing medium style (In case it matters, I set the Image style to use in the content for the first image the same - medium) I set the Image style to use in the Colorbox as the original image. There are no images in the content - it is not finding the medium style images, if there is a title added to the image, it is visible and clicking on it brings up the full size image. I looked in the files and the medium size images do exist. Firebug shows the src path to one folder above where the files are - and no file img width="220" height="165" title="example image" alt="example image" src="http://localhost/example/files/styles/medium/public" It should be http://localhost/example/files/styles/medium/public/RealEstate/example.jpg Hope the feedback helps.

biolithic commented 9 years ago

Hi, thanks for the feedback. I did a simple check of creating a node with an image field and Colorbox seemed to work on that. I will try your example to see how I can better make this to work for you.

On Thu, May 14, 2015 at 4:32 PM, krbennett notifications@github.com wrote:

I tried the new your new version. Better, but I still have problems. I set the Image style to use in the content to the existing medium style (In case it matters, I set the Image style to use in the content for the first image the same - medium) I set the Image style to use in the Colorbox as the original image. There are no images in the content - it is not finding the medium style images, if there is a title added to the image, it is visible and clicking on it brings up the full size image. I looked in the files and the medium size images do exist. Firebug shows the src path to one folder above where the files are - and no file img width="220" height="165" title="example image" alt="example image" src="http://localhost/example/files/styles/medium/public" It should be http://localhost/example/files/styles/medium/public/RealEstate/example.jpg Hope the feedback helps.

— Reply to this email directly or view it on GitHub https://github.com/backdrop-contrib/colorbox/issues/1#issuecomment-102176681 .

serundeputy commented 9 years ago

Hi.

thanks for the great work @biolithic !

I can verify @krbennett 's findings.

I've poked at the code and can not isolate where the path is being set when using the colorbox field format. The colorbox works great! fires and displays the image; just when viewing the node that the img src is incorrect.

screen shot 2015-06-27 at 11 23 23 am

in the colorbox correct src attribute: screen shot 2015-06-27 at 11 30 02 am

@quicksketch any insight? I'd be happy to do the leg work, fix and file a PR, but as I said I'm not finding where the img src attribute is being set.

thanks!

quicksketch commented 9 years ago

I'm not finding where the img src attribute is being set.

I'm pretty sure it's going to be in theme_colorbox_image_formatter(): https://github.com/backdrop-contrib/colorbox/blob/1.x-1.x/colorbox.theme.inc#L19

serundeputy commented 9 years ago

hrm ... i can set the alt attribute, but I can not seem to set or effect the src attribute.

$image = theme('image', array('src' => $variables['image']['path'], 'alt' => 'el_geffe;'));
serundeputy commented 9 years ago

i'm sorry; :hushed: https://github.com/backdrop-contrib/colorbox/pull/2

this is a terrible, terrible hack:

again sorry; I don't think you should merge this, but it provides a patch if others need to get going for now.

biolithic commented 9 years ago

Hi, Thanks for filing this. I'm trying to finish up my June blog tonight so that I can get onto other things like this (hopefully tomorrow!) Will look at it/merge etc when I get that done. Thanks again

On Sat, Jun 27, 2015 at 6:11 PM, Geoff St. Pierre notifications@github.com wrote:

i'm sorry; [image: :hushed:]

2 https://github.com/backdrop-contrib/colorbox/pull/2

this is a terrible, terrible hack:

  • maybe it will inspire someone to find the right way
  • get's me going for now

again sorry; I don't think you should merge this, but it provides a patch if others need to get going for now.

— Reply to this email directly or view it on GitHub https://github.com/backdrop-contrib/colorbox/issues/1#issuecomment-116164944 .

serundeputy commented 9 years ago

@biolithic i don't think you should merge it ... just have it available as a patch until a suitable fix can be found. https://github.com/serundeputy/colorbox/commit/ccfe5da5240272dd42618d86ade632fb6504542f.patch thanks, ~Geoff

docwilmot commented 9 years ago

theme_image_style() expects a uri key for the image, colorbox is providing a path key. Change in colorbox.theme.inc: line 27: from 'path' => $item['uri'] to 'uri' => $item['uri'] line 127 to $path = image_style_url($style_name, $image['uri']); line 130 to $path = file_create_url($image['uri']);

Sorry I'd do a patch but expecting dinner guests.

serundeputy commented 9 years ago

thanks @docwilmot !

that works for me; PR : https://github.com/backdrop-contrib/colorbox/pull/3

thanks @biolithic , @quicksketch and @docwilmot for your attention and help on this!

~Geoff