On Wed, Feb 16, 2011 at 1:24 PM, Marius Dumitru Florea <
mariusdumitru.flo...@xwiki.com> wrote:
> Hi Jerome,
>
> On 02/16/2011 01:37 PM, Jerome Velociter wrote:
> > On Wed, Feb 16, 2011 at 12:12 PM, mflorea
> > <platform-notificati...@xwiki.org>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"><</div><div class="next"
> >> title="Show next image">></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.
>
I'm curious to know the rationale behind your reasoning.
I see mostly advantages in preferring prototype over native APIs :
* Browser issues can be tricky, I personally would not assume that I always
know when to use prototype to avoid one. I admit I don't test my code in all
browsers (Konqueror, Opera, etc. you name it.), but I know prototype.js is
extensively tested including in exotic browsers.
* Using prototype.js selectors you should get the best of breed of browsers
selectors, so overall the performance is better. When you use
getElementsByTagName I guess you assume it runs faster than prototype
selector. Maybe it's true today (but I would think not by far) but I will
pretty likely not be tomorrow (and let's hope by far) - when most browsers
will have a good implementation of querySelectorAll (like Opera has for
example) ; which prototype selector will certainly rely on (when it's
available) in future versions (if not already, haven't checked).
The advantage of using prototype.js here is the same argument of having a
unified API and hiding implementation details. Native JS APIs is
unfortunately always a moving target, since browsers implement
specifications at their own pace, not in the same order, etc. Only a using a
library's API you can abstract this and be pretty safe your code will run
fine everywhere and with maximized performance.
* Where do you place the bar ? Why $('element') simplifies your code over
.getElementById('element') but not .select('img') over
.getElementsByTagName('img')
* RE my argument about not extended elements
I won't make a proposal to enforce that, but again I'm curious to hear what
you think.
Jerome.
> >
> >
> >> + 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
> >> notificati...@xwiki.org
> >> http://lists.xwiki.org/mailman/listinfo/notifications
> >>
> > _______________________________________________
> > devs mailing list
> > devs@xwiki.org
> > http://lists.xwiki.org/mailman/listinfo/devs
> _______________________________________________
> devs mailing list
> devs@xwiki.org
> http://lists.xwiki.org/mailman/listinfo/devs
>
_______________________________________________
devs mailing list
devs@xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
