Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#12002 closed enhancement (fixed)

[patch] [ccla] Toolbar widget is necessary

Reported by: ykami Owned by: Shane O'Sullivan
Priority: high Milestone: 1.6
Component: DojoX Mobile Version: 1.5
Keywords: Cc: Jared Jurkiewicz
Blocked By: Blocking:

Description

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.

Attachments (3)

12002.patch (39.0 KB) - added by Douglas Hays 9 years ago.
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.
12002icons.zip (2.7 KB) - added by Douglas Hays 9 years ago.
New icons from Yoshiroh Kamiyama for dojox/mobile/themes/domButtons/compat/.
12002-revised.patch (38.8 KB) - added by bill 9 years ago.
revised patch from Kamiyama-san

Download all attachments as: .zip

Change History (14)

Changed 9 years ago by Douglas Hays

Attachment: 12002.patch added

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.

Changed 9 years ago by Douglas Hays

Attachment: 12002icons.zip added

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

comment:1 Changed 9 years ago by Douglas Hays

Cc: Jared Jurkiewicz added

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

comment:2 Changed 9 years ago by bill

Summary: Toolbar widget is necessary[patch] [ccla] Toolbar widget is necessary

comment:3 Changed 9 years ago by 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

comment:4 Changed 9 years ago by 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?

comment:5 Changed 9 years ago by 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);

comment:6 Changed 9 years ago by 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?

comment:7 Changed 9 years ago by 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.

Changed 9 years ago by bill

Attachment: 12002-revised.patch added

revised patch from Kamiyama-san

comment:8 Changed 9 years ago by bill

I attached the revised patch from Kamiyama-san.

comment:9 Changed 9 years ago by 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

comment:10 Changed 9 years ago by Shane O'Sullivan

Resolution: fixed
Status: newclosed

Proxy commit for Kamiyama-san

comment:11 Changed 8 years ago by bill

Milestone: tbd1.6

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

Note: See TracTickets for help on using tickets.