Ryan Hellyer

10 Responses to “Simple Colorbox version 1.5”

1. 

   Milan says:

   I must be honest with you and tell you that I actually don’t like changes you’ve made, and I’ll explain it in more detail below.

   But first I want to explain what was background behind my proposal. I needed Colorbox feature but I needed it with things your plugin didn’t have: conditional loading (eg. only when galleries are used; I planned to publish code I use when you release new version), translations of labels and a few custom options.

   At first I created simple function that handled this, but then I needed it again, and again… So I figured it’s time to actually improve upstream and then use filters or child classes for specific needs.

   Whats wrong with your changes? Too much filters! You have 50 filters for options for what could have been used just one. Passing array to filter is standard in WordPress core, in bbPress they made it even simpler with bbp_parse_args(). The idea is to change everything you need in one pass and to make code as lightweight as possible.

   I’ll show you with one example how complicated using many filters is. Lets say I want to change 3 random options. With my code it would look like:

   
function md_filter_simple_colorbox_options( $options ) {
$options['scalePhotos'] = 'false';
$options['width'] = '50%';
$options['transition'] = 'none';

   return $options;
}
add_filter( 'simple_colorbox_settings', 'md_filter_simple_colorbox_options' );


   With your code it would look like:

   
function md_filter_simple_colorbox_width( $option ) {
return '50%';
}
add_filter( 'simple_colorbox_width', 'md_filter_simple_colorbox_width' );

   function md_filter_simple_colorbox_transition( $option ) {
   return 'none';
   }
   add_filter( 'simple_colorbox_transition', 'md_filter_simple_colorbox_transition' );

   function md_filter_simple_colorbox_scalephotos( $option ) {
return 'false';
}
add_filter( 'simple_colorbox_scalephotos', 'md_filter_simple_colorbox_scalephotos' );


   Consequence of passing all default values that are already defined in Colorbox is that there is unnecessary large inline script and unnecessary additional file request.

   Also, there is a reason why I used context and placeholders for some strings, and why texdomain is loaded on init.

   Finally, I believe it’s better to keep methods name intact for backward compatibility as someone might use this class, in my patch I considered leaving empty Simple_Colorbox::inline_scripts() just because of that.

   We really made too much talk about thing that should be simple. 🙂

   August 19, 2013 at 3:16 pm # // Reply

   • 

     Ryan says:

     Dang, that really is a lot better :/

     I was thinking that it was an exorbitant number of filters to be adding and I was concerned about the added bloat to the inline code, but for some reason your solution did not dawn on me 🙁

     I’ll fix it up some time this week and post an update. Thanks for the feedback 🙂

     August 19, 2013 at 8:58 pm # // Reply

   • 

     Ryan says:

     I’m sitting here quite frustrated, lol. I should have realised this whilst I was writing the code :/

     August 19, 2013 at 9:02 pm # // Reply

     • 

       Milan says:

       Trust me, just apply my patch and start from there. If there is anything more you want to change, do that from that version. 😉

       August 19, 2013 at 9:53 pm # // Reply

       • 

         Ryan says:

         I wasn’t aware of any patch.

         August 19, 2013 at 10:39 pm # // Reply

       • 

         Ryan says:

         Arghhh, darn it 🙁 I did all of this when I was recovering from a day of no sleep. I guess that led to me not reading your ticket correctly at all 🙁 This is a mess. I had no idea you had provided a patch.

         August 19, 2013 at 10:42 pm # // Reply

         • 

           Milan says:

           Sorry man, I thought it was obvious. 🙁

           August 20, 2013 at 11:59 am # //

         • 

           Ryan says:

           That was just me being an idiot. I decided to work on this as I was too tired to do much else. I guess choosing the most tired time to work on something is not the smartest idea :/

           August 20, 2013 at 2:25 pm # //

       • 

         Ryan says:

         I haven’t forgotten about this BTW. I’m just waiting until I have some more free time to sit down and do it properly and hopefully not bork it up again :/

         August 22, 2013 at 1:26 pm # // Reply

         • 

           Milan says:

           No worries. This isn’t urgent at all.

           August 23, 2013 at 11:54 am # //