Hi Jerome,

On 02/16/2011 01:37 PM, Jerome Velociter wrote:
> On Wed, Feb 16, 2011 at 12:12 PM, mflorea
> <[email protected]>wrote:
>
>> Author: mflorea
>> Date: 2011-02-16 12:12:34 +0100 (Wed, 16 Feb 2011)
>> New Revision: 34727
>>

[snip]

>> Added:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.js
>> ===================================================================
>> ---
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.js
>>                                (rev 0)
>> +++
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.js
>>        2011-02-16 11:12:34 UTC (rev 34727)
>> @@ -0,0 +1,139 @@
>> +var XWiki = (function (XWiki) {
>> +// Start XWiki augmentation.
>> +XWiki.Gallery = Class.create({
>> +  initialize : function(container) {
>> +    this.images = this._collectImages(container);
>> +
>> +    this.container = container.update('<input type="text" tabindex="-1"
>> class="focusCatcher"/><div class="currentImageWrapper"><img
>> class="currentImage" alt="Current image"/></div><div class="navigation"><div
>> class="previous" title="Show previous image">&lt;</div><div class="next"
>> title="Show next image">&gt;</div><div style="clear:both"></div></div><div
>> class="index">0 / 0</div><div class="maximize" title="Maximize"></div>');
>>
>

> Titles will not be localized here

Right. Thanks for catching this. I forgot about it.

>
> +    this.container.addClassName('xGallery');
>> +
>> +    this.focusCatcher = this.container.down('.focusCatcher');
>> +    this.focusCatcher.observe('keydown',
>> this._onKeyDown.bindAsEventListener(this));
>> +    this.container.observe('click', function() {
>> +      this.focusCatcher.focus();
>> +    }.bind(this));
>> +
>> +    this.container.down('.previous').observe('click',
>> this._onPreviousImage.bind(this));
>> +    this.container.down('.next').observe('click',
>> this._onNextImage.bind(this));
>> +
>> +    this.currentImage = this.container.down('.currentImage');
>> +    this.currentImage.observe('load', this._onLoadImage.bind(this));
>> +    this.currentImage.observe('error', this._onErrorImage.bind(this));
>> +    this.currentImage.observe('abort', this._onAbortImage.bind(this));
>> +
>> +    this.indexDisplay = this.container.down('.index');
>> +
>> +    this.maximizeToggle = this.container.down('.maximize');
>> +    this.maximizeToggle.observe('click',
>> this._onToggleMaximize.bind(this));
>> +
>> +    this.show(0);
>> +  },
>> +  _collectImages : function(container) {
>> +    var images = [];
>> +    var imageElements = container.getElementsByTagName('img');
>>
>

> Why not use container.select('img') ?

[snip]

>> +  _updateSize : function() {
>> +    var dimensions = document.viewport.getDimensions();
>> +    // Remove container padding;
>> +    var width = dimensions.width - 20;
>> +    var height = dimensions.height - 20;
>> +    this.container.style.width = width + 'px';
>> +    this.container.style.height = height + 'px';
>> +    this.currentImage.parentNode.style.height = height + 'px';
>> +    this.currentImage.parentNode.style.lineHeight = height + 'px';
>>
>
> I think as a best practice we should prefer prototype.js accessors versus. a
> mix of prototype helpers and native DOM properties. It's more consistent and
> more bug-proof in my opinion. I tend to always assume elements have been
> extended by prototype, but when the code mixes prototype.js and native
> accessors, you have greater chances to introduce non-extended elements as
> variables / members of your class that another developer will maybe miss.
>
> Here you would do
>
> this.currentImage.up().setStyle({
>    lineHeight: height + 'px'
> })

I prefer to use native JavaScript APIs whenever possible and use 
Prototype only when:
* it simplifies my code
* it fixes a cross browser issue.

You can make a proposal but I'd be -0 for using Prototype in any situation.

>
>
>> +    this.currentImage.style.maxHeight = height + 'px';
>> +    // Remove width reserved for the navigation arrows.
>> +    this.currentImage.style.maxWidth = (width - 128) + 'px';
>> +  },
>> +  _resetSize : function() {
>> +    this.container.style.cssText = '';
>> +    this.container.removeAttribute('style');
>> +    this.currentImage.parentNode.style.cssText = '';
>> +    this.currentImage.parentNode.removeAttribute('style');
>> +    this.currentImage.style.cssText = '';
>> +    this.currentImage.removeAttribute('style');
>> +  },
>> +  show : function(index) {
>> +    if (index<  0 || index>= this.images.length || index == this.index) {
>> +      return;
>> +    }
>> +    this.currentImage.style.visibility = 'hidden';
>> +    Element.addClassName(this.currentImage.parentNode, 'loading');
>> +    this.currentImage.title = this.images[index].title;
>> +    this.currentImage.src = this.images[index].url;
>> +    this.index = index;
>> +    this.indexDisplay.firstChild.nodeValue = (index + 1) + ' / ' +
>> this.images.length;
>>
>
> Same for firstChild.nodeValue, I would prefer .down().update()
>
>
>> +  }
>> +});
>> +// End XWiki augmentation.
>> +return XWiki;
>> +}(XWiki || {}));
>> +
>> +Element.observe(document, "dom:loaded", function() {
>> +  $$('.gallery').each(function(gallery) {
>>
>

> Same remark as for the dashboard macro, IMO this is too greedy and
> $mainContentArea.select('.gallery') would perform better.
>
> The problem is not really each such selector taken individually, but if we
> continue adding more and more, it will make the loading after DOM is loaded
> clumsy.

I agree, but I don't like the fact that gallery.js would depend on the 
presence of the mainContentArea element. Is this element part of the 
public "API"?

Thanks,
Marius

>
> Jerome.
>
>
>
>> +    new XWiki.Gallery(gallery);
>> +  });
>> +});
>>
>>
>> Property changes on:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/gallery.js
>> ___________________________________________________________________
>> Added: svn:keywords
>>    + Author Id Revision HeadURL
>> Added: svn:eol-style
>>    + native
>>
>> Added:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/loading.gif
>> ===================================================================
>> (Binary files differ)
>>
>>
>> Property changes on:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/loading.gif
>> ___________________________________________________________________
>> Added: svn:mime-type
>>    + image/gif
>>
>> Added:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/maximize.gif
>> ===================================================================
>> (Binary files differ)
>>
>>
>> Property changes on:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/maximize.gif
>> ___________________________________________________________________
>> Added: svn:mime-type
>>    + image/gif
>>
>> Added:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/minimize.gif
>> ===================================================================
>> (Binary files differ)
>>
>>
>> Property changes on:
>> platform/web/trunk/standard/src/main/webapp/resources/uicomponents/widgets/gallery/minimize.gif
>> ___________________________________________________________________
>> Added: svn:mime-type
>>    + image/gif
>>
>> _______________________________________________
>> notifications mailing list
>> [email protected]
>> http://lists.xwiki.org/mailman/listinfo/notifications
>>
> _______________________________________________
> devs mailing list
> [email protected]
> http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs

Reply via email to