16 Feb
2011
16 Feb
'11
2:06 p.m.
On Wed, Feb 16, 2011 at 1:24 PM, Marius Dumitru Florea <
mariusdumitru.florea(a)xwiki.com> wrote:
Hi Jerome,
On 02/16/2011 01:37 PM, Jerome Velociter wrote:
> I think as a best practice we should prefer prototype.js accessors
versus. a
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.
On Wed, Feb 16, 2011 at 12:12 PM, mflorea
<platform-notifications(a)xwiki.org>wrote;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';
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
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
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
notifications(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/notifications
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs