Author: nbelaevski Date: 2007-06-03 16:36:52 -0400 (Sun, 03 Jun 2007) New Revision: 988 Modified: trunk/sandbox-samples/panelmenu-sample/ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuGroupRenderer.java trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuItemRenderer.java trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRenderer.java trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRendererBase.java trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/iconImages/PanelMenuIconArrow.java trunk/sandbox/panelmenu/src/main/resources/org/richfaces/renderkit/html/scripts/panelMenu.js trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenu.jspx trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenuGroup.jspx Log: Code review notes commited Modified: trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuGroupRenderer.java =================================================================== --- trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuGroupRenderer.java 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuGroupRenderer.java 2007-06-03 20:36:52 UTC (rev 988) @@ -38,6 +38,7 @@ public class PanelMenuGroupRenderer extends PanelMenuRendererBase { protected Class getComponentClass() { + //TODO by nick - dima - add concrete component class return UIComponent.class; } @@ -72,6 +73,9 @@ String align = ""; UIPanelMenu panelMenu = findMenu(component); + //TODO by nick - dima - add isTopLevel() method to group component + //TODO by nick - dima - btw, components can be enclosed in other components such as ui:repeat, so panelManu will be + // parent of parent of ... boolean isTopLevel = component.getParent() instanceof UIPanelMenu; if (isTopLevel){ @@ -96,12 +100,16 @@ return; } + //TODO by nick - dima - add isTopLevel() method to group component boolean isTopLevel = component.getParent() instanceof UIPanelMenu; + //TODO by nick - dima - component value should be used here boolean isOpened = isOpened(context,component).equals("opened") ? true : false; String source = ""; String iconNode = ""; + //TODO by nick - dima - access through attributes map is usually slower as done through reflection. Should be replaced with abstract + // getter invocation Object iconCollapsed = component.getAttributes().get("iconCollapsed"); Object iconExpanded = component.getAttributes().get("iconExpanded"); String icon = (isOpened ? iconExpanded : iconCollapsed).toString(); @@ -117,6 +125,7 @@ int h = 16; //width(context); StringBuffer imageBuffer = new StringBuffer(); + //TODO by nick - dima - use writer.startElement()/writer.endElement()/getUtils().writeAttribute() calls to output image imageBuffer.append("<img src=\"" + source + "\" "); imageBuffer.append("alt=\"\" vspace=\"0\" hspace=\"0\" "); imageBuffer.append("style=\"display:block;\" "); @@ -164,6 +173,7 @@ format.add("display", "none"); if(component.getParent() instanceof UIPanelMenuGroup) { UIPanelMenuGroup parent = (UIPanelMenuGroup)component.getParent(); + //TODO by nick - dima - take opened value from component itself PanelMenuGroupRenderer renderer = (PanelMenuGroupRenderer) context.getRenderKit().getRenderer(parent.getFamily(), parent.getRendererType()); try { if ( renderer.isOpened(context, parent).equals("opened") ){ Modified: trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuItemRenderer.java =================================================================== --- trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuItemRenderer.java 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuItemRenderer.java 2007-06-03 20:36:52 UTC (rev 988) @@ -40,6 +40,7 @@ public class PanelMenuItemRenderer extends PanelMenuRendererBase { protected Class getComponentClass() { + //TODO by nick - dima - add concrete component class return UIComponent.class; } @@ -72,6 +73,7 @@ return; } String align = ""; + //TODO by nick - dima - add isTopLevel() method to item component boolean isTopLevel = component.getParent() instanceof UIPanelMenu; if (isTopLevel){ @@ -119,6 +121,7 @@ if (!iconType.equals("none")){ int h = 16; //width(context); + //TODO by nick - dima - use writer.startElement()/writer.endElement()/getUtils().writeAttribute() calls to output image StringBuffer imageBuffer = new StringBuffer(); imageBuffer.append("<img src=\"" + source + "\" "); imageBuffer.append("alt=\"\" vspace=\"0\" hspace=\"0\" "); @@ -162,6 +165,7 @@ } + //TODO by nick - dima - looks like duplicate code methods. can we add it to super class? public String getHideStyle(FacesContext context, UIComponent component) { // TODO Auto-generated method stub if (!(component.getParent() instanceof UIPanelMenu)) { Modified: trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRenderer.java =================================================================== --- trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRenderer.java 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRenderer.java 2007-06-03 20:36:52 UTC (rev 988) @@ -41,16 +41,13 @@ import org.richfaces.component.panelmenu.UIPanelMenuItem; import org.richfaces.renderkit.iconImages.PanelMenuIconSpacer; -/** - * @author hans - * - */ public class PanelMenuRenderer extends PanelMenuRendererBase { /* (non-Javadoc) * @see org.ajax4jsf.framework.renderer.RendererBase#getComponentClass() */ protected Class getComponentClass() { + //TODO by nick - dima - add concrete component class return UIComponent.class; } @@ -132,6 +129,7 @@ .append("',parentId:'") .append((String) child.getParent().getClientId(context)) .append("'},{type:" + (child instanceof UIPanelMenuItem ? "\"item\"":"\"node\"")) + //TODO by nick - dima - use "".equals(onopen) .append(",onopen:"+(onopen.equals("") ? "\"\"" : "\"" + onopen + "\"")+",onclose:"+(onclose.equals("") ? "\"\"" : "\"" + onclose + "\"")) .append(",event:\"" + event + "\"") .append(",mode:\"" + mode + "\"") @@ -248,6 +246,7 @@ } private void addActionIfNeeded(FacesContext context,UIComponent child,StringBuffer buffer){ + //TODO by nick - dima - use CommandScriptBuilder if (child instanceof UIPanelMenuItem){ if (child.getChildCount()>0){ buffer.append(",'panelMenuItemAction'"); Modified: trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRendererBase.java =================================================================== --- trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRendererBase.java 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/PanelMenuRendererBase.java 2007-06-03 20:36:52 UTC (rev 988) @@ -59,6 +59,7 @@ StringBuffer buffer = new StringBuffer(); String src = getIconByType("custom",false,context,component); int w = 16; //width(context); + //TODO by nick - dima - use writer.startElement()/writer.endElement()/getUtils().writeAttribute() calls to output image for (int i=0;i<level;i++){ buffer.append("<td><img src=\"" + src + "\" alt=\"\" hspace=\"0\" vspace=\"0\" height=\""+w+"\" width=\""+w+"\" style=\"display:block;\" /></td>\n"); } @@ -73,6 +74,7 @@ return level; } parent = parent.getParent(); + //TODO by nick - dima - inc level counter only if parent is panelMenu(Group) component level++; } return level; @@ -99,6 +101,7 @@ color = (String) skin.getParameter(context,"panelmenu.itemBulletColor"); } if(iconType != null && !iconType.equals("none")){ + //TODO by nick - dima - why not call image classes by their names in doc. Eg. not PanelMenuIconArrowUp, but PanelMenuChevronImage if (iconType.equals("custom")){ source = getResource(PanelMenuIconSpacer.class.getName()).getUri(context, color); } else if (iconType.equals("triangle")) { @@ -120,6 +123,7 @@ } else if (iconType.equals("grid")) { source = getResource(PanelMenuIconDots.class.getName()).getUri(context, color); } else { + //TODO by nick - dima - TemplateContext is deprecated and shouldn't be used source = (String)getUtils().encodeResourceURL(new TemplateContext(this,context,component),iconType); } } @@ -226,8 +230,10 @@ protected String getItemMode(UIComponent component) { String parentExpandMode = findMenu(component).getExpandMode(); String parentMode = findMenu(component).getMode(); + //TODO by nick - dima - always evaluates to false if (null == parentMode && "".equals(parentMode)) parentMode = "server"; + //TODO by nick - dima - always evaluates to false if (null == parentExpandMode && "".equals(parentExpandMode)) parentExpandMode = "none"; String mode = "none"; Modified: trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/iconImages/PanelMenuIconArrow.java =================================================================== --- trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/iconImages/PanelMenuIconArrow.java 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/java/org/richfaces/renderkit/iconImages/PanelMenuIconArrow.java 2007-06-03 20:36:52 UTC (rev 988) @@ -37,6 +37,7 @@ public class PanelMenuIconArrow extends PanelMenuIconBasic { public PanelMenuIconArrow() { + //TODO by nick - dima - move the code to super class constructor setRenderer(new GifRenderer()); setLastModified(new Date(InternetResourceBuilder.getInstance().getStartTime())); } Modified: trunk/sandbox/panelmenu/src/main/resources/org/richfaces/renderkit/html/scripts/panelMenu.js =================================================================== --- trunk/sandbox/panelmenu/src/main/resources/org/richfaces/renderkit/html/scripts/panelMenu.js 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/resources/org/richfaces/renderkit/html/scripts/panelMenu.js 2007-06-03 20:36:52 UTC (rev 988) @@ -6,6 +6,7 @@ window.Richfaces = {}; } +//TODO by nick - dima - better not to use global object to track panels var PanelMenuStorage = new Object(); PanelMenu = Class.create(); @@ -183,6 +184,7 @@ if (this.action !="panelMenuItemAction"){ this.preTrigger(e); var form = Event.findElement(e, "form"); + //TODO by nick - dima - no need to create form if(!form || typeof(form) == 'undefined' || !form.tagName || form.tagName.toLowerCase() != 'form'){ form = document.createElement('form'); form.setAttribute('method', 'post'); Modified: trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenu.jspx =================================================================== --- trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenu.jspx 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenu.jspx 2007-06-03 20:36:52 UTC (rev 988) @@ -30,10 +30,12 @@ <tbody> <vcp:body> + <!-- TODO by nick - dima - vcp:body content is fully ignored --> <f:call name="renderChildren" /> </vcp:body> <tr style="display:none"> <td> + <!-- TODO by nick - dima - no need for clientId here --> <f:clientid var="clientId"/> <f:call name="insertScript"/> </td> Modified: trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenuGroup.jspx =================================================================== --- trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenuGroup.jspx 2007-06-02 01:45:10 UTC (rev 987) +++ trunk/sandbox/panelmenu/src/main/templates/org/richfaces/htmlPanelMenuGroup.jspx 2007-06-03 20:36:52 UTC (rev 988) @@ -12,6 +12,7 @@ <f:clientid var="clientId"/> + <!-- TODO by nick - dima - use #{clientId}tdhide instead of tdhide#{clientId} --> <tr id="tdhide#{clientId}" style="#{this:getHideStyle(context, component)}" > <td> <jsp:scriptlet> @@ -31,20 +32,24 @@ <f:call name="utils.encodeAttributes"> <f:parameter value="onclick,onmousedown,onmouseup,onmousemove" /> </f:call> + <!-- TODO by nick - dima - encodeAttributes & encodePassThru conflict --> <f:call name="utils.encodePassThru" /> <tr> + <!-- TODO by nick - dima - just id="#{clientId}" --> <f:call name="utils.encodeId" /> <f:call name="insertTDs" /> <td> <f:call name="insertImage"> <f:parameter value="left" /> </f:call> - </td> + </td> + <!-- TODO by nick - dima - item_content should be removed --> <td style="width:100%" class="item_content" id="icon#{clientId}" > <input type="hidden" name="panelMenuState#{clientId}" value="#{this:isOpened(context, component)}" > - </input> + </input> + <!-- TODO by nick - dima - one input should be enough --> <input type="hidden" name="panelMenuAction#{clientId}" value="" > </input> @@ -61,6 +66,7 @@ </td> </tr> <vcp:body> + <!-- TODO by nick - dima - vcp:body content is fully ignored --> <f:call name="renderChildren" /> </vcp:body> </f:root> \ No newline at end of file Property changes on: trunk/sandbox-samples/panelmenu-sample ___________________________________________________________________ Name: svn:ignore + target