Last modified: 2012-02-23 16:33:26 UTC
Created attachment 10014 [details] SRF_Gallery.php patch SRF_Gallery.php produced the following error: Notice: Undefined property: StubObject::$mOutput in ...\extensions\SemanticResultFormats\Gallery\SRF_Gallery.php on line 166 Fatal error: Call to a member function addImage() on a non-object in ...\extensions\SemanticResultFormats\Gallery\SRF_Gallery.php on line 166 Patch tested on: Semantic Result Formats (Version 1.7); Semantic MediaWiki (Version 1.7.0.2); MediaWiki 1.18.0; PHP 5.3.8; MySQL 5.5.16
I applied this as part of your patch at https://github.com/mwjames/smw-srfgallerycarousel This looks a bit odd to me though: !$imgTitle->getDBkey() == null I read that as: if the negation of $imgTitle->getDBkey() is 'sort of equal' to null. Is that what you want it to do? Please use is_null().
Created attachment 10058 [details] Patch on SRF_Gallery.php Hope this works, the patch includes: * Use is_null() instead and since I'm bit lazy I just put in some other changes related to SRF_Gallery.php * Tweak css so vertical display is similar to horizontal css * Use perrow parameter as indicator for the amount of visible and scrollable elements * Prepare other optional paramters that are not implemented yet * The jcarousel developer suggested to use wrap=both instead of wrap=circular as default
Created attachment 10061 [details] SRF_Gallery.php Patch 0.1.2, includes 0.1.1 plus RTL support This patch includes all above plus * RTL CSS support
Can you please use tabs to indent like done in these files already? I fixed the indentation last time, but am not going to keep doing that. Looks good apart from that.
Created attachment 10065 [details] Fix indentation OK, this should fix the indentation for all required files including the latest change from r112069
Created attachment 10079 [details] SRF_Gallery-Carousel-0.1.3.patch for multi instances display Ah, hope this is the last patch but I finally got the multi-instance display working which means that with 0.1.3 more than one carousel can be displayed per page, the display of a standard gallery combined with gallery carousel works as supposed to be. All previous changes are included and tested with Chrome and Firefox
Great. Applied in r112201, and tested with trunk :) Some comments though: * Please provide patches against trunk. If someone makes a change on SVN and you just incorporate it in your patch, merging will result in a conflict since the code is already there. * You where doing some false selection in the JS :) * If you have a dictionary of stuff in JS (ie { stuff: ... }), then don't put a comma after the last element, this makes IE cry (yeah, really >_>) * Avoid aligning assignments in most cases, can be useful in setup files with long lists of similar assignments, but rarely in other places. One improvement possible in the JS is getting rid of all the var and just assigning directly in the dictionary, like this: ( '#carousel' ).jcarousel( { parseInt($( '#carousel' ).attr( 'scroll' ) ), ...
Ahhh! Bad timing :) Can you provide a patch against trunk? :/
Created attachment 10080 [details] Version 0.1.3, patch against r112205 Let's see if we hit the bullseye this time
r112215 * Please don't prefix stuff with 'm_' or 'm' * Code style convention for both JS and PHP is to have spaces between round brackets if there is something in between, so .attr( "id" ) rather then .attr("id") I made some changes to the PHP (since the merge failed anyway... looking foreward to git :). The style mismatch is really trivial, no need to make another patch for that. This script allows you to mass fix style issues btw: http://svn.wikimedia.org/viewvc/mediawiki/trunk/tools/code-utils/stylize.php?view=co