У чет, 14. 04 2011. у 13:22 +0000, Gavin Panella пише:
> Review: Approve
> Wow, the subscriptions stuff is looking *awesome* these days.

Thanks. It's all the yellow-goo over it :)
Though, we are building on the sound foundation you guys did
previously :)

> This branch looks good. I have a few comments, but nothing major.
> 
> 
> [1]
> 
> +        var header_link = Y.Node.create('<a></a>')
> +            .set('href', '#')
> 
> Is this needed? Ah, maybe for IE?

Not needed at all, other than the mouse pointer doesn't turn into a
"click me" pointer.  Basically, the <a> tag is considered an anchor
instead of a link.  I am thinking of keeping it, any better suggestions?

> 
> [2]
> 
> +        Y.Assert.areNotSame(
> +            undefined,
> 
> Y.Assert.isNotUndefined() is slightly more concise.

Ah, thanks. Fixed.

> 
> [3]
> 
> +        var node = module._get_other_descriptions_node(info);
> +        var link = node.one('#other-subscriptions-header a');
> +        var list = node.one('#other-subscriptions-list');
> 
> IDs are /meant/ to be unique in the page so you could skip the first
> line above and just use Y.one().

Except that the stuff is not added to the page DOM yet.  The method
returns a node which other methods are in charge of adding to the page:
and since this is trying to be more of a unit test, I wouldn't want to
add it.

> [4]
> 
> +        this.wait(function() {
> [...]
> +            this.wait(function() {
> [...]
> +            }, 500);
> +        }, 500);
> 
> Are these delays long enough? How long is the animation meant to take?

They consistently worked for me (and anything lower consistently failed
for me).  It's taking whatever the default is for animation.  I'll check
what it is and use that + some small leverage.

> 
> [5]
> 
> -              ss_module.setup_bug_subscriptions(
> -                  {content_box: "#structural-subscription-content-box"});
>                info_module.show_subscription_description(
>                    {description_box: "#subscription-description-box"});
>            });
> +          Y.on('contentready', function() {
> +              ss_module.setup_bug_subscriptions(
> +                  {content_box: "#other-subscriptions-list"});
> +          }, '#other-subscriptions-list');
> 
> http://developer.yahoo.com/yui/3/examples/event/event-timing.html
> seems to suggest that it's not safe to modify the DOM until domready
> fires:
> 
>     The 'contentready' event fired for the element
>     'contentContainer'. That element and all of its children are
>     present in the DOM.
> 
>     The 'domready' event fired. The DOM is now safe to modify via
>     script.
> 
> setup_bug_subscriptions() calls at least one function that modifies
> the DOM, so I suspect this change should be reverted.

Well, it's put inside 'contentready' for #other-subscriptions-list,
which is constructed in show_subscription_description, which is called
in 'domready' event.  '#other-subscriptions-list' is *not* present in
the DOM before show_subscription_description() runs.

> 
> [6]
> 
> +    content_node = content_node.one('#overlay-container');
> 
> Like before, and in a few other places, you can just use Y.one() here,
> not that it matters much.
> 
> Becauase IDs are, afaik, meant to be unique in a DOM tree, I dislike
> them; they're like the global variables of the DOM, and I try to avoid
> using them, especially in widgets. Do you have any opinions either
> way, or are you aware of anyone else's?

Well, I use them exactly where I want elements to be unique in a page: I
actually prefer that over using classes.  That forces people to think
carefully before adding more of the same (iow, what I did for these
elements with IDs was to design them so they are the only ones in a
page). I am hoping the browser/YUI can optimize the search better if I
do a look-up inside a containing node.

Atm, this is a "widget" designed for a single page only.

Also, since DOM standard doesn't specify a way to get elements using CSS
selectors (though, there are querySelector() and
querySelectorAll()), .one() can't fallback on something very fast like
document.getElementByID(). So, I am helping YUI/browsers narrow the
search at least a bit down.

Perhaps it doesn't help much (I can imagine browsers using
well-performing hashes), but I feel better about it. :)

Cheers,
Danilo