[patch] [ccla] Toolbar widget is necessary

[ykami]

The heading widget should be able to have various toolbar buttons. And it should be able to be used as a header bar or a footer bar.

[Douglas Hays]

Patch from Yoshiroh Kamiyama (IBM, CCLA). Updated the Heading widget so that it can place various buttons on it. Updated the TabBar? widget so that it can be placed on the Heading widget. Added the ToolBarButton? widget. Added support for DOM buttons.

[Douglas Hays]

New icons from Yoshiroh Kamiyama for dojox/mobile/themes/domButtons/compat/.

[Douglas Hays]

Jared/Shane?, please review, and commit if warranted.

[Shane O'Sullivan]

Change TabBar? code to use dojo.style to save bytes

this.domNode.style.background = "none"; this.domNode.style.border = "none"; this.domNode.style.width = totalW + 2 + "px";

becomes

dojo.style(this.domNode, {

background: 'none', border: 'none', width: (totalW + 2) + "px"

});

Even better, move the background and border into CSS, and just apply the dynamic style (width) programmatically

In _base.js, the check for Blackberry is invalid. BB 6 uses webkit

Line 349: hard coding in 21 pixels offset is a bad idea. Move this to CSS so it can be overridded per theme.

Line 367: This seems like an extremely arbitrary decision, "if the label is longer than x, do something". The size depends on the font and size of the text, which can change per customer. Can't you measure the size of the dom node containing the label?

Line 626: Use dojo.hasClass()

Line 650: transitionTo now takes 4 arguments

Line 186: Shorter to set a variable called color, e.g.

var color; if(x){color = a;} else if(y){color = b;} dojo.addClass(this.domNode, color);

Line 1422: use dojo.toggleClass

[ykami]

Thanks Shane for the code review.
I couldn't find relevant code around line 186 and 1422. Probably we can't trust the line numbers any longer. Could you give me a context (a few lines of code) for both?

[Shane O'Sullivan]

Apologies:

Line 186 should be line 1386

On line 1422 of your version of _base.js, replace

if(deselect){ // deselect 
   dojo.removeClass(this.domNode, this._selColor); 
}else{ // select 
  dojo.addClass(this.domNode, this._selColor); 
} 

with

dojo.toggleClass(this.domNode, this._selColor, !deselect);

[ykami]

Shane,

dojo.style(this.domNode, {

background: 'none', border: 'none', width: (totalW + 2) + "px"

});

I agree, thanks.

In _base.js, the check for Blackberry is invalid. BB 6 uses webkit

The isBB flag is for non-webkit BB browsers. For the BB 6 browser, isWebKit becomes true, and thus isBB becomes false. It is the expected result.

Line 349: hard coding in 21 pixels offset is a bad idea. Move this to CSS so it can be overridded per theme.

I agree. Let me fix it as follows. (One line added, two lines modified)

            this._body = body;
            this._head = head; <-- added this line
            this._btn = btn;
            body.innerHTML = this.back;
            this.connect(body, "onclick", "onClick");
            var neck = dojo.create("DIV", {className:"mblArrowButtonNeck"}, btn);
            btn.style.width = body.offsetWidth + head.offsetWidth + "px"; <-- modified this line
            this.setLabel(this.label);
        }
    },

    startup: function(){
        if(this._btn){
            this._btn.style.width = this._body.offsetWidth + this._head.offsetWidth + "px"; <- modified this line
        }
    },

Line 367: This seems like an extremely arbitrary decision

I agree. Let me simply remove this line.

this.domNode.style.textAlign = this.label.length > 12 ? "left" : "";

Line 626: Use dojo.hasClass()

Here I couldn't use dojo.hasClass() since the pattern is regexp.

Line 650: transitionTo now takes 4 arguments

Oh, I didn't know that. Just adding this.scene is fine?

Line 186: Shorter to set a variable called color, e.g.

Do you mean something like this?
It doesn't look shorter, but either way is fine for me.

var color;
if(this.selected){
    color = this._selColor;
}else if(this.domNode.className.indexOf("mblColor") == -1){
    color = this._defaultColor;
}
dojo.addClass(this.domNode, color);

dojo.toggleClass(this.domNode, this._selColor, !deselect);

I agree.

Are you able to commit the patch with those changes? Should I recreate the patch?

[Shane O'Sullivan]

It would make it a lot simpler if you could submit another patch thanks.

Regarding the color code, it is a little shorter and will compress down more than your original code because of the use of local variables.

[bill]

revised patch from Kamiyama-san

[bill]

I attached the revised patch from Kamiyama-san.

[Shane O'Sullivan]

(In [23421]) Refs #12002 Adds the ability for a mobile Heading widget to have multiple buttons, not just a back button !strict

[Shane O'Sullivan]

Proxy commit for Kamiyama-san

[bill]

bulk update: bugs fixed in past few months, presumably milestone is 1.6