feat #276 custom image for Iconbutton #324

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@smadapathi

This pull request allows users to provide their own icon image for Iconbutton widget

@jakub-g jakub-g commented on an outdated diff Jan 18, 2013
test/aria/templates/issue276/IconButtonTestCase.js
+ runTemplateTest : function () {
+ this.__checkIconButtonProperties();
+ this.__finishTest();
+ },
+ __checkIconButtonProperties : function () {
+ var iconBtnId, domUtil = aria.utils.Dom, iconBtnElement, width, height, imageURL, imagename;
+ iconBtnId = this.getWidgetInstance("myid")._domId + "_iconbtn";
+ iconBtnElement = aria.utils.Dom.getElementById(iconBtnId);
+ width = domUtil.getStyle(iconBtnElement, "width");
+ height = domUtil.getStyle(iconBtnElement, "height");
+ // Background url is different for different browsers, so just checking image name
+ imageURL = domUtil.getStyle(iconBtnElement, "background").split(' ')[0].replace(/("|')/g, "");
+ imagename = imageURL.substring(imageURL.lastIndexOf('/') + 1, imageURL.lastIndexOf(')'));
+ this.assertTrue(width == "42px", "Width is not proper");
+ this.assertTrue(height == "16px", "Height is not proper");
+ this.assertTrue(imagename == "icon-check.png", "Image is not proper");
@jakub-g
jakub-g Jan 18, 2013 collaborator

It's better to use assertEquals(a, b, msg) instead of assertTrue (you can then easier see the actual in expected value, as they'll be available in the stack trace).

@jakub-g jakub-g commented on the diff Jan 18, 2013
test/aria/templates/issue276/IconButtonTestCase.js
@@ -0,0 +1,39 @@
+Aria.classDefinition({
+ $classpath : "test.aria.templates.issue276.IconButtonTestCase",
@jakub-g
jakub-g Jan 18, 2013 collaborator

You've created a test case file but haven't added it to any test suite. Please reference this test in TemplatesTestSuite.js.

@jakub-g jakub-g commented on an outdated diff Jan 18, 2013
src/aria/widgets/action/IconButton.js
@@ -48,7 +55,61 @@ Aria.classDefinition({
* @private
*/
_widgetMarkupContent : function (out) {
- this._icon.writeMarkup(out);
+ var cfg = this._cfg, id = this._domId+"_iconbtn", tooltip = cfg.tooltip, iconInfo, sourceImage = cfg.sourceImage;
@jakub-g
jakub-g Jan 18, 2013 collaborator

It's better to split this into multiple lines for readability. Also for better experience with debuggers (although here's it's not the big deal as we read from cfg mostly, but this holds especially when invoking functions).

@jakub-g jakub-g commented on an outdated diff Jan 18, 2013
src/aria/widgets/action/IconButton.js
@@ -48,7 +55,61 @@ Aria.classDefinition({
* @private
*/
_widgetMarkupContent : function (out) {
- this._icon.writeMarkup(out);
+ var cfg = this._cfg, id = this._domId+"_iconbtn", tooltip = cfg.tooltip, iconInfo, sourceImage = cfg.sourceImage;
+ if (sourceImage) {
+ iconInfo = {
@jakub-g
jakub-g Jan 18, 2013 collaborator

I'd remove iconInfo completely from line 58 and put var here at the beginning.

Generally (it's a matter of personal taste though) I prefer to put var in the smallest possible scope instead of declaring all the vars at the beginning of the function -- with the notable exception like this:

var a, b;
if (something) { 
  a = 1;
  b = 2;
}
else {
  a = 3;
  b = 4;
}
@jakub-g jakub-g commented on an outdated diff Jan 18, 2013
test/aria/templates/issue276/IconButtonTestCase.js
+ this.setTestEnv({
+ template : "test.aria.templates.issue276.TemplateIconBtn"
+ });
+
+ },
+
+ $prototype : {
+ tearDown : function () {
+ aria.core.IO.$unregisterListeners(this);
+ },
+ runTemplateTest : function () {
+ this.__checkIconButtonProperties();
+ this.__finishTest();
+ },
+ __checkIconButtonProperties : function () {
+ var iconBtnId, domUtil = aria.utils.Dom, iconBtnElement, width, height, imageURL, imagename;
@jakub-g
jakub-g Jan 18, 2013 collaborator

As mentioned in the other place, I would put var just before each assignment to a variable, since the code here is linear, without if-s. Also, some newlines (e.g. before the assertions) will make the code more pleasant to read.

@jakub-g jakub-g and 1 other commented on an outdated diff Jan 18, 2013
src/aria/widgets/action/IconButton.js
+ cssClasses += " xBlock"
+ }
+ return cssClasses;
+ },
+ /**
+ * Return the icon style for a given icon
+ * @param {Object} iconInfo
+ * @return {String}
+ */
+ _getIconStyle : function (iconInfo) {
+ var cfg = this._cfg;
+ var vAlign = !iconInfo.verticalAlign ? "" : "vertical-align: " + iconInfo.verticalAlign;
+ var margins = "margin: 0 0 0 0 "; // default value
+
+ if (cfg.margins != null && cfg.margins.match(/^(\d+|x) (\d+|x) (\d+|x) (\d+|x)$/)) {
+ var margArray = cfg.margins.split(" ");
@jakub-g
jakub-g Jan 18, 2013 collaborator

If you rename margArray to some shorter identifier, the next assignment will fit in one line. Since margArray is a local variable and is supposed to be used just in the next line, inside this short if, its name is not extremely important. You could name it even m or sth like this.

@piuccio
piuccio Jan 18, 2013

Please don't use m, in non minified code names are meant to be read by humans, so please choose a meaningful name.

@piuccio
piuccio commented Jan 24, 2013

I don't really understand why this is not done at Icon level.
IconButton is simply a button creating an Icon widget inside. We could improve both widgets reusing the same code.

Apparenlty you just copied and pasted the code from Icon.writeMarkup. This is bad both because the code is duplicated but also because you're doing more than what's needed:

  • Why are you handling the tooltip?
  • Why are you adding the cfg.margins on the image, those are for the button.
@piuccio piuccio commented on an outdated diff Jan 24, 2013
test/aria/templates/issue276/TemplateIconBtn.tpl
@@ -0,0 +1,19 @@
+{Template {
+ $classpath:'test.aria.templates.issue276.TemplateIconBtn',
+ $hasScript:false
+}}
+
+ {macro main()}
+
+ {@aria:IconButton {
+ id:"myid",
+ sourceImage:{ path:"test/aria/templates/issue276/icon-check.png", width:42},
@piuccio
piuccio Jan 24, 2013

This path is relative, your test might fail depending on where files and images are deployed.

Please use

aria.core.DownloadMgr.resolveURL("test/aria/templates/issue276/icon-check.png")

instead. It'll give you the absolute path computed accordingly to where your test is.

@piuccio piuccio commented on an outdated diff Jan 24, 2013
test/aria/templates/issue276/IconButtonTestCase.js
@@ -0,0 +1,40 @@
+Aria.classDefinition({
+ $classpath : "test.aria.templates.issue276.IconButtonTestCase",
+ $extends : "aria.jsunit.TemplateTestCase",
+ $constructor : function () {
+ this.$TemplateTestCase.constructor.call(this);
+
+ this.setTestEnv({
+ template : "test.aria.templates.issue276.TemplateIconBtn"
+ });
+
+ },
+
+ $prototype : {
+ tearDown : function () {
+ aria.core.IO.$unregisterListeners(this);
@piuccio
piuccio Jan 24, 2013

Damn copy/paste! What's this?

@piuccio
piuccio commented Jan 30, 2013

Integrated in fc797d0

@piuccio piuccio closed this Jan 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment