#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)
Change History (14)
Changed 10 years ago by
Attachment: | 12002.patch added |
---|
Changed 10 years ago by
Attachment: | 12002icons.zip added |
---|
New icons from Yoshiroh Kamiyama for dojox/mobile/themes/domButtons/compat/.
comment:1 Changed 10 years ago by
Cc: | Jared Jurkiewicz added |
---|
Jared/Shane?, please review, and commit if warranted.
comment:2 Changed 10 years ago by
Summary: | Toolbar widget is necessary → [patch] [ccla] Toolbar widget is necessary |
---|
comment:3 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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.
comment:9 Changed 10 years ago by
comment:10 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Proxy commit for Kamiyama-san
comment:11 Changed 10 years ago by
Milestone: | tbd → 1.6 |
---|
bulk update: bugs fixed in past few months, presumably milestone is 1.6
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.