SproutCore--over 20000 lines of new code!
in [JavaScript]
|
From: David Mark on 31 Dec 2009 03:19 After 20000 lines of new code, over 5000 new unit tests, and countless hours of effort by over 30 contributors, SproutCore 1.0 is ready. ... Is it? First suspicious file is browser.js (wonder what that could be?) // ========================================================================== // Project: SproutCore - JavaScript Application Framework // Copyright: ©2006-2009 Sprout Systems, Inc. and contributors. // Portions ©2008-2009 Apple Inc. All rights reserved. // License: Licened under MIT license (see license.js) // ========================================================================== Hmm. Portions copyright Apple, Inc. Big company == Great JS. /** Detects the current browser type. Borrowed from jQuery + prototype */ Oh no. SC.browser = (function() { var userAgent = navigator.userAgent.toLowerCase(); var version = (userAgent.match( /.+(?:rv|it|ra|ie)[\/: ]([\d.]+)/ ) || [])[1] ; var browser = /** @scope SC.browser */ { Please, make it stop. :) Scope? /** The current browser version */ version: version, /** non-zero if webkit-based browser */ safari: (/webkit/).test( userAgent ) ? version : 0, Non-zero? This must be their own unique interpretation of jQuery's sniffing. I wonder if they realize that jQuery moved forward one step to object inferences. /** non-zero if this is an opera-based browser */ opera: (/opera/).test( userAgent ) ? version : 0, The one browser in the world that is easy to detect and they blew it. Of course, there should be no reason to detect Opera in 2010 (or 2009 or 2008...) Can only go downhill from here. /** non-zero if this is IE */ msie: (/msie/).test( userAgent ) && !(/opera/).test( userAgent ) ? version : 0, Sad, especially considering IE has conditional comments. Oh, but that won't fit into their plans of creating a way-cool browser scripting framework. :) /** non-zero if this is a miozilla based browser */ Miozilla? Not very polished is it? mozilla: (/mozilla/).test( userAgent ) && !(/(compatible| webkit)/).test( userAgent ) ? version : 0, /** non-zero if this is mobile safari */ mobileSafari: (/apple.*mobile.*safari/).test(userAgent) ? version : 0, /** non-zero if we are on windows */ windows: !!(/(windows)/).test(userAgent), /** non-zero if we are on a mac */ mac: !!((/(macintosh)/).test(userAgent) || (/(mac os x)/).test (userAgent)), language: (navigator.language || navigator.browserLanguage).split ('-', 1)[0] }; // Add more SC-like descriptions... Huh? Comments that don't make sense are worse than no comments at all. SC.extend(browser, /** @scope SC.browser */ { isOpera: !!browser.opera, isIe: !!browser.msie, isIE: !!browser.msie, isSafari: !!browser.safari, isMobileSafari: !!browser.mobileSafari, isMozilla: !!browser.mozilla, isWindows: !!browser.windows, isMac: !!browser.mac, /** The current browser name. This is useful for switch statements. */ How so? current: browser.msie ? 'msie' : browser.mozilla ? 'mozilla' : browser.safari ? 'safari' : browser.opera ? 'opera' : 'unknown' }) ; return browser ; })(); I don't even want to look in the next file. How about... core_query.js. Sounds promising. :) // ========================================================================== // Project: SproutCore - JavaScript Application Framework // Copyright: ©2006-2009 Sprout Systems, Inc. and contributors. // Portions ©2008-2009 Apple Inc. All rights reserved. // License: Licened under MIT license (see license.js) // ========================================================================== /*globals CQ add*/ require('system/builder') ; /** CoreQuery is a simplified DOM manipulation library used internally by SproutCore to find and edit DOM elements. Outside of SproutCore, you should generally use a more full-featured DOM library such as Prototype or jQuery. In other words, abandon all hope... How is it that all of these various JS projects turn out with the same sort nonsense inside them. The logic (and lack thereof), the comments, the documentation, the forum posts, etc. are always so far distanced from reality; and yet, the justifications for them always reference some Real World where it makes sense to be ignorant, incompetent, apathetic and lazy. Oh, and grandiose. :) Why can't these people find another language to screw up? "CoreQuery itself is a subset of jQuery with some additional plugins. If you have jQuery already loaded when SproutCore loads, in fact, it will replace CoreQuery with the full jQuery library and install any CoreQuery plugins, including support for the SC.Enumerable mixin." Well, this is going to be a very short review... "Much of this code is adapted from jQuery 1.2.6, which is available under an MIT license just like SproutCore." ....and history lesson. ::) var styleFloat = SC.browser.msie ? "styleFloat" : "cssFloat"; I remember that one. It's pretty much Game Over as everything else in the ZIP sits atop this. // A helper method for determining if an element's values are broken Again, worthless comments are worse than worthless. var styleIsBorked = function styleIsBorked( elem ) { Oh, a named function expression. Somebody let the developers out of the sound-proof booth. They need to make contact with the rest of the world. if ( !SC.browser.safari ) return false; So, only "values" in Safari (or whatever this thing thinks is Safari) can be "borked". I really dislike cutesy geek-speak like that. It's another layer of cheese (on top of a moldy jQuery circa 2007). // defaultView is cached Again with the silly comments. var ret = defaultView.getComputedStyle( elem, null ); return !ret || ret.getPropertyValue("color") === ""; } ; // implement core methods here from jQuery that we want available all the // time. Use this area to implement jQuery-compatible methods ONLY. // New methods should be added at the bottom of the file, where they will // be installed as plugins on CoreQuery or jQuery. CQ = CoreQuery = SC.Builder.create( /** @scope SC.CoreQuery.fn */ { Whatever. Skipping to bottom... // Add some global helper methods. SC.mixin(SC.$, { /** @private helper method to determine if an element is visible. Exposed for use in testing. */ isVisible: function(elem) { var CQ = SC.$; return ("hidden"!=elem.type) && (CQ.css(elem,"display")!="none") && (CQ.css(elem,"visibility")!="hidden"); } }) ; I've seen that pattern before (and it never fails to make me cringe). What on earth do hidden inputs have to do with elements that have been styled hidden (or removed from the layout?) The developers can't see them? Thanks for bottling that guys. :) for(var key in enumerable) { if (!enumerable.hasOwnProperty(key)) continue ; Oops, just lost Safari 2. It's not that it's necessary to support all older browsers, but you can't just cut them off in the middle of a script with an exception. I assume the developers didn't realize what they were doing (usually a fair assumption). for ( name in options ) { CQ.attr( (type)?this.style:this, name, CQ.prop( this, options[ name ], type, i, name )); } No filter on that one? The attr method is the usual suspect from jQuery. Hard to believe anyone would copy it at this point. Of course, it's hard to believe anyone would still be using it at this late date. Whatever. // Check to see if the W3C box model is being used boxModel: !SC.browser.msie || document.compatMode === "CSS1Compat", That was in jQuery until they "punted" box models (along with quirks mode in general). isCoreQuery: YES, // walk like a duck That's about enough I think. Over 30 people wasted a lot of time building widgets on top of this rubbish heap. The 5000 unit tests are obviously worthless. I could have told them all they needed to know without writing a single one. Executive summary: forget it. Obviously nobody is ever going to go back in here and rewrite this jQuery regurgitation (and they sure as hell can't "upgrade" it either). So they might as well turn out the lights and padlock the place. The world didn't need a parallel jQuery universe (and sure as hell not another jQuery UI). ;) And how do I know the author(s) will find this review "superficial". In reality, I gave it far more time than it deserved. Guys, find another hobby. I mean that in the kindest possible sense. :)
From: Charles Jolley on 1 Jan 2010 00:47 Hi I am the creator of sproutcore. If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext. Also I think nitpicking little bits of code like this is really pointless. Anyone who has written a big project knows that some parts receive more attention then others based on their actual real world needs. Picking one remote part like this and disecting it misses all to useful stuff you might have found if you has just looked. For example spend some time looking at the datastore or observer layers --- frmsrcurl: http://compgroups.net/comp.lang.javascript/SproutCore-over-20000-lines-of-new-code
From: David Mark on 1 Jan 2010 00:56 On Jan 1, 12:47 am, Charles Jolley <u...(a)compgroups.net/> wrote: > Hi I am the creator of sproutcore. If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext. Very often? Why is an old version of jQuery (!) in there at all? And the comments indicate it will use an extant jQuery if found. That's suicide as newer versions are incompatible. So good luck with that! > > Also I think nitpicking little bits of code like this is really pointless.. Anyone who has written a big project knows that some parts receive more attention then others based on their actual real world needs. Oh, the "real world" argument? Here's a thought: the lowest level DOM code is everything for a project like this. Don't build castles in a swamp. ;) > Picking one remote part like this and disecting it misses all to useful stuff you might have found if you has just looked. Remote part? It's deep in the core. > > For example spend some time looking at the datastore or observer layers Is it worth it? What do those layers sit upon? And Happy New Year! :)
From: David Mark on 1 Jan 2010 01:31 On Jan 1, 12:47 am, Charles Jolley <u...(a)compgroups.net/> wrote: > Hi I am the creator of sproutcore. If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext. > // ========================================================================== // Project: SproutCore - JavaScript Application Framework // Copyright: ©2006-2009 Sprout Systems, Inc. and contributors. // Portions ©2008-2009 Apple Inc. All rights reserved. // License: Licened under MIT license (see license.js) // ========================================================================== sc_require('system/builder'); /** set update mode on context to replace content (preferred) */ SC.MODE_REPLACE = 'replace'; /** set update mode on context to append content */ SC.MODE_APPEND = 'append'; /** set update mode on context to prepend content */ SC.MODE_PREPEND = 'prepend'; /** @namespace A RenderContext is a builder that can be used to generate HTML for views or to update an existing element. Rather than making changes to an element directly, you use a RenderContext to queue up changes to the element, finally applying those changes or rendering the new element when you are finished. You will not usually create a render context yourself but you will be passed a render context as the first parameter of your render() method on custom views. Render contexts are essentially arrays of strings. You can add a string to the context by calling push(). You can retrieve the entire array as a single string using join(). This is basically the way the context is used for views. You are passed a render context and expected to add strings of HTML to the context like a normal array. Later, the context will be joined into a single string and converted into real HTML for display on screen. In addition to the core push and join methods, the render context also supports some extra methods that make it easy to build tags. context.begin() <-- begins a new tag context context.end() <-- ends the tag context... */ SC.RenderContext = SC.Builder.create(/** SC.RenderContext.fn */ { SELF_CLOSING: SC.CoreSet.create().addEach('area base basefront br hr input img link meta'.w()), basefont (and it is long deprecated) /** When you create a context you should pass either a tag name or an element that should be used as the basis for building the context. If you pass an element, then the element will be inspected for class names, styles and other attributes. You can also call update() or replace() to modify the element with you context contents. If you do not pass any parameters, then we assume the tag name is 'div'. A second parameter, parentContext, is used internally for chaining. You should never pass a second argument. @param {String|DOMElement} tagNameOrElement @returns {SC.RenderContext} receiver */ init: function(tagNameOrElement, prevContext) { if (tagNameOrElement === undefined) tagNameOrElement = 'div'; // if a prevContext was passed, setup with that first... if (prevContext) { this.prevObject = prevContext ; this.strings = prevContext.strings ; this.offset = prevContext.length + prevContext.offset ; } if (!this.strings) this.strings = [] ; // if tagName is string, just setup for rendering new tagName this.needsContent = YES ; if (SC.typeOf(tagNameOrElement) === SC.T_STRING) { this._tagName = tagNameOrElement.toLowerCase(); this._needsTag = YES ; // used to determine if end() needs to wrap tag // increase length of all contexts to leave space for opening tag var c = this; while(c) { c.length++; c = c.prevObject; } this.strings.push(null); this._selfClosing = this.SELF_CLOSING.contains(this._tagName); } else { this._elem = tagNameOrElement ; this._needsTag = NO ; this.length = 0 ; this.needsContent = NO ; } return this ; }, // .......................................................... // PROPERTIES // // NOTE: We store this as an actual array of strings so that browsers that // support dense arrays will use them. /** The current working array of strings. @property {Array} */ strings: null, /** this initial offset into the strings array where this context instance has its opening tag. @property {Number} */ offset: 0, /** the current number of strings owned by the context, including the opening tag. @property {Number} */ length: 0, /** Specify the method that should be used to update content on the element. In almost all cases you want to replace the content. Very carefully managed code (such as in CollectionView) can append or prepend content instead. You probably do not want to change this propery unless you know what you are doing. @property {String} */ updateMode: SC.MODE_REPLACE, /** YES if the context needs its content filled in, not just its outer attributes edited. This will be set to YES anytime you push strings into the context or if you don't create it with an element to start with. */ needsContent: NO, // .......................................................... // CORE STRING API // /** Returns the string at the designated index. If you do not pass anything returns the string array. This index is an offset from the start of the strings owned by this context. @param {Number} idx the index @returns {String|Array} */ get: function(idx) { var strings = this.strings || []; return (idx === undefined) ? strings.slice(this.offset, this.length) : strings[idx+this.offset]; }, /** Adds a string to the render context for later joining. Note that you can pass multiple arguments to this method and each item will be pushed. @param {String} line the liene to add to the string. Liene? @returns {SC.RenderContext} receiver */ push: function(line) { var strings = this.strings, len = arguments.length; if (!strings) this.strings = strings = []; // create array lazily if (len > 1) { strings.push.apply(strings, arguments) ; } else strings.push(line); // adjust string length for context and all parents... var c = this; while(c) { c.length += len; c = c.prevObject; } this.needsContent = YES; return this; }, /** Pushes the passed string onto the array, but first escapes the string to ensure that no user-entered HTML is processed as HTML. @param {String} line one or mroe lines of text to add @returns {SC.RenderContext} receiver */ text: function(line) { var len = arguments.length, idx=0; for(idx=0;idx<len;idx++) { this.push(SC.RenderContext.escapeHTML(arguments[idx])); } return this ; }, /** Joins the strings together, returning the result. But first, this will end any open tags. @param {String} joinChar optional string to use in joins. def empty string @returns {String} joined string */ join: function(joinChar) { // generate tag if needed... if (this._needsTag) this.end(); var strings = this.strings; return strings ? strings.join(joinChar || '') : '' ; }, // .......................................................... // GENERATING // /** Begins a new render context based on the passed tagName or element. Generate said context using end(). @returns {SC.RenderContext} new context */ begin: function(tagNameOrElement) { // console.log('%@.begin(%@) called'.fmt(this, tagNameOrElement)); Delete debugging code. return SC.RenderContext(tagNameOrElement, this); }, /** If the current context targets an element, this method returns the element. If the context does not target an element, this method will render the context into an offscreen element and return it. @returns {DOMElement} the element */ element: function() { if (this._elem) return this._elem; // create factory div if needed var ret, child; if (!SC.RenderContext.factory) { SC.RenderContext.factory = document.createElement('div'); } SC.RenderContext.factory.innerHTML = this.join(); // In IE something weird happens when reusing the same element. That doesn't inspire confidence. ;) // After setting innerHTML, the innerHTML of the element in the previous view // turns blank. Seems that there is something weird with their garbage // collection algorithm. No. There is something weird going on with your script(s). // I tried just removing the nodes after keeping a // reference to the first child, but it didnt work. // Ended up cloning the first child. http://www.jibbering.com/faq/faq_notes/clj_posts.html#ps1DontWork if(SC.RenderContext.factory.innerHTML.length>0){ child = SC.RenderContext.factory.firstChild.cloneNode(true); SC.RenderContext.factory.innerHTML = ''; } else { child = null; } return child ; }, /** Removes an element with the passed id in the currently managed element. */ remove: function(elementId) { // console.log('remove('+elementId+')'); if (!elementId) return ; Delete the above two lines. If the calling code is broken, let it break so you can see the problem(s). ;) var el, elem = this._elem ; if (!elem || !elem.removeChild) return ; Huh? No removeChild method featured, so fail silently? el = document.getElementById(elementId) ; if (el) { el = elem.removeChild(el) ; el = null; } }, /** If an element was set on this context when it was created, this method will actually apply any changes to the element itself. If you have not written any inner html into the context, then the innerHTML of the element will not be changed, otherwise it will be replaced with the new innerHTML. Also, any attributes, id, classNames or styles you've set will be updated as well. This also ends the editing context session and cleans up. @returns {SC.RenderContext} previous context or null if top */ update: function() { var elem = this._elem, mode = this.updateMode, key, value, styles, factory, cur, next, before; this._innerHTMLReplaced = NO; if (!elem) { // throw "Cannot update context because there is no source element"; return ; *Sigh* See above. } // console.log('%@#update() called'.fmt(this)); // if (this.length>0) console.log(this.join()); // else console.log('<no length>'); // replace innerHTML if (this.length>0) { this._innerHTMLReplaced = YES; if (mode === SC.MODE_REPLACE) { elem.innerHTML = this.join(); } else { factory = elem.cloneNode(false); factory.innerHTML = this.join() ; before = (mode === SC.MODE_APPEND) ? null : elem.firstChild; cur = factory.firstChild ; while(cur) { next = cur.nextSibling ; elem.insertBefore(cur, next); cur = next ; } cur = next = factory = before = null ; // cleanup } } // note: each of the items below only apply if the private variable has // been set to something other than null (indicating they were used at // some point during the build) // if we have attrs, apply them if (this._attrsDidChange && (value = this._attrs)) { for(key in value) { if (!value.hasOwnProperty(key)) continue; You just sank Safari 2. See previous post. if (value[key] === null) { // remove empty attrs elem.removeAttribute(key); LOL. You are making the same mistakes as jQuery, Prototype, YUI, etc. http://www.cinsoft.net/attributes.html } else { SC.$(elem).attr(key, value[key]); You can't be a little pregnant. Thank you very much for wasting my time. :)
From: Garrett Smith on 1 Jan 2010 02:07
David Mark wrote: > After 20000 lines of new code, over 5000 new unit tests, and countless > hours of effort by over 30 contributors, SproutCore 1.0 is ready. ... > > Is it? > > First suspicious file is browser.js (wonder what that could be?) > [...] > /** Detects the current browser type. Borrowed from jQuery + prototype > */ > Th idea of parsing navigator.userAgent is faulty inference. > > Oh no. > > > SC.browser = (function() { > > var userAgent = navigator.userAgent.toLowerCase(); > var version = (userAgent.match( /.+(?:rv|it|ra|ie)[\/: ]([\d.]+)/ ) > || [])[1] ; > > var browser = /** @scope SC.browser */ { > > > Please, make it stop. :) Scope? > @Scope is a JS-doc comment. I did argue with Michael Matthews about that. JS-Doc toolkit comments are a pain in hte neck to keep up to date and just add clutter to the actual code. But anyway, that is why that comment xists. > /** The current browser version */ > version: version, > > /** non-zero if webkit-based browser */ > safari: (/webkit/).test( userAgent ) ? version : 0, > That is a wrongly named identifier. "webkit" is not safari. Webkit is on Palm pre, in Chrome, in iris. > > Non-zero? This must be their own unique interpretation of jQuery's > sniffing. I wonder if they realize that jQuery moved forward one step > to object inferences. > > > /** non-zero if this is an opera-based browser */ > opera: (/opera/).test( userAgent ) ? version : 0, > > > The one browser in the world that is easy to detect and they blew it. > Of course, there should be no reason to detect Opera in 2010 (or 2009 > or 2008...) Can only go downhill from here. > > > /** non-zero if this is IE */ > msie: (/msie/).test( userAgent ) && !(/opera/).test( userAgent ) ? > version : 0, > MSIE is a very commonly faked user-agent. Recognizing that Opera masks as "msie" should have been a hint that this might not be a good idea. > > Sad, especially considering IE has conditional comments. Oh, but that > won't fit into their plans of creating a way-cool browser scripting > framework. :) > Conditional comments would be much more accurate here. ScriptEngine would, too, though cc would be best. > mozilla: (/mozilla/).test( userAgent ) && !(/(compatible| > webkit)/).test( userAgent ) ? version : 0, > It is less error-prone to use 'Gecko'. Applications that embed Mozilla should not be using 'mozilla' in the userAgent, but would be likely to have 'Gecko' in the user-agent. Regardless, this whole browser detection inference is faulty. > /** non-zero if this is mobile safari */ > mobileSafari: (/apple.*mobile.*safari/).test(userAgent) ? > version : 0, > > /** non-zero if we are on windows */ > windows: !!(/(windows)/).test(userAgent), > > /** non-zero if we are on a mac */ > mac: !!((/(macintosh)/).test(userAgent) || (/(mac os x)/).test > (userAgent)), > That should not be something the program would care about. A user platform is not something that HTTP includes and might very well be excluded from an embedded system. > language: (navigator.language || navigator.browserLanguage).split > ('-', 1)[0] > }; > > // Add more SC-like descriptions... > > > Huh? Comments that don't make sense are worse than no comments at > all. > That is a senseless comment. [...] > > > current: browser.msie ? 'msie' : browser.mozilla ? 'mozilla' : > browser.safari ? 'safari' : browser.opera ? 'opera' : 'unknown' > It looks like four browsers. There are more than four browsers available on the market and many of these are going to sadly fall into the category of "unknown". > }) ; > > return browser ; > > })(); > > > I don't even want to look in the next file. How about... > core_query.js. Sounds promising. :) > > > // > ========================================================================== > // Project: SproutCore - JavaScript Application Framework > // Copyright: ©2006-2009 Sprout Systems, Inc. and contributors. > // Portions ©2008-2009 Apple Inc. All rights reserved. > // License: Licened under MIT license (see license.js) > // > ========================================================================== > /*globals CQ add*/ > > require('system/builder') ; > > /** > CoreQuery is a simplified DOM manipulation library used internally > by SproutCore to find and edit DOM elements. Outside of SproutCore, > you should generally use a more full-featured DOM library such as > Prototype or jQuery. > That is a useless comment. Useless because it does not state what features are lacking and what these libraries should be used for. > > "CoreQuery itself is a subset of jQuery with some additional plugins. > If you have jQuery already loaded when SproutCore loads, in fact, it > will replace CoreQuery with the full jQuery library and install any > CoreQuery plugins, including support for the SC.Enumerable mixin." > > > Well, this is going to be a very short review... > > > "Much of this code is adapted from jQuery 1.2.6, which is available > under an MIT license just like SproutCore." > jQuery 1.2 has a lot of obvious bugs that were fixed. It should not be used at this time. > > ...and history lesson. ::) > > > var styleFloat = SC.browser.msie ? "styleFloat" : "cssFloat"; > > I remember that one. It's pretty much Game Over as everything else in > the ZIP sits atop this. > The problem with this inference is that it ties browser that has 'msie' in userAgent to using 'styleFloat' and browsers that do not use 'cssFloat'. Where available, the standard cssFloat property should be used. It is very easy to detect:- var FLOAT_PROP = "cssFloat"; if(document.documentElement.style[FLOAT_PROP] === "undefined") { FLOAT_PROP = "styleFloat"; } This assumes that cssFloat is supported, and if it isn't, then it uses styleFloat. > // A helper method for determining if an element's values are broken > > > Again, worthless comments are worse than worthless. > > > var styleIsBorked = function styleIsBorked( elem ) { > Best to not repeat feature tests; perform the feature test only once. > > Oh, a named function expression. Somebody let the developers out of > the sound-proof booth. They need to make contact with the rest of the > world. > There is no good reason to not use a FunctionDeclaration there. > > if ( !SC.browser.safari ) return false; > > > So, only "values" in Safari (or whatever this thing thinks is Safari) > can be "borked". I really dislike cutesy geek-speak like that. It's > another layer of cheese (on top of a moldy jQuery circa 2007). > It isn't just Safari, remember, but *any* webkit here. Only a browser that identifies as webkit may be borked. > > // defaultView is cached > > > Again with the silly comments. > > > var ret = defaultView.getComputedStyle( elem, null ); > return !ret || ret.getPropertyValue("color") === ""; > } ; > The method call to `getPropertyValue("color") can be replaced with ret.color. > > // implement core methods here from jQuery that we want available all > the > // time. Use this area to implement jQuery-compatible methods ONLY. > // New methods should be added at the bottom of the file, where they > will > // be installed as plugins on CoreQuery or jQuery. > CQ = CoreQuery = SC.Builder.create( /** @scope SC.CoreQuery.fn */ { > That looks like an assignment to an undeclared identifier. The identifiers should have been declared using `var`. > > // Add some global helper methods. > SC.mixin(SC.$, { > The identifier `$` is not descriptive. It should be renamed to describe what it means. > /** @private helper method to determine if an element is visible. > Exposed yes, the method is exposed. > for use in testing. */ > isVisible: function(elem) { > var CQ = SC.$; > return ("hidden"!=elem.type) && (CQ.css(elem,"display")!="none") > && (CQ.css(elem,"visibility")!="hidden"); > } > > }) ; It might be worth considering the case for visibility: collapse. > > > I've seen that pattern before (and it never fails to make me cringe). > What on earth do hidden inputs have to do with elements that have been > styled hidden (or removed from the layout?) The developers can't see > them? Thanks for bottling that guys. :) > > > for(var key in enumerable) { > > if (!enumerable.hasOwnProperty(key)) continue ; > > > Oops, just lost Safari 2. It's not that it's necessary to support all > older browsers, but you can't just cut them off in the middle of a > script with an exception. I assume the developers didn't realize what > they were doing (usually a fair assumption). > > > for ( name in options ) { > CQ.attr( > (type)?this.style:this, > name, CQ.prop( this, options[ name ], type, i, name )); > } > > > No filter on that one? The attr method is the usual suspect from > jQuery. Hard to believe anyone would copy it at this point. Of > course, it's hard to believe anyone would still be using it at this > late date. Whatever. > I have no idea what that method is intended to perform, but if it is using jQuery.attr, it is obviously in very poor judgement, as is everything else here. > > // Check to see if the W3C box model is being used > boxModel: !SC.browser.msie || document.compatMode === > "CSS1Compat", > > > That was in jQuery until they "punted" box models (along with quirks > mode in general). > > isCoreQuery: YES, // walk like a duck > > > That's about enough I think. Over 30 people wasted a lot of time > building widgets on top of this rubbish heap. Sounds like Dojo. This library is not going to die. -- Garrett comp.lang.javascript FAQ: http://jibbering.com/faq/ |