Posted by
Jacopo Cappellato on
Jan 09, 2007; 6:10am
URL: http://ofbiz.116.s1.nabble.com/Please-review-the-attached-patch-fro-the-HtmlFormRenderer-class-tp176227p176235.html
Adrian, all,
first of all a very minor point:
in your patch there are many lines changed just to suppress unnecessary
blanks; this is fine but it could hide the real mods you did and make
the review a bit more complex... I hope that this will not refrain the
other committers, and most of all David Jones, that is the master of
widgets :-) to read your patch and comment.
Second, about the refactoring, here is what I think:
1) it's fine to move all the styling info (as you suggested in your
comments) to the css
2) when we have to specify a non-standard style property (such as
different align properties for the th elements discussed before) we
should move them to a css class and hardcode the class name if one is
not passed; for example:
if (UtilValidate.isNotEmpty(titleStyle)) {
buffer.append(" class=\"");
buffer.append(titleStyle);
buffer.append("\"");
}
should be changed to:
if (UtilValidate.isNotEmpty(titleStyle)) {
buffer.append(" class=\"");
buffer.append(titleStyle);
buffer.append("\"");
} else {
buffer.append(" class=\"defaultTitleStyle\"");
}
3) as a general rule, if the html element should be rendered with the
default OFBiz style, there is no need to specify the hardcoded class as
done in #2
4) as a future step: we should consider to change the single form html
from table-based to div-based
Jacopo
Adrian Crum wrote:
> Jacopo,
>
> I've attached a patch for the HtmlFormRenderer class that has your patch
> applied. I have also added comments on what changes need to be made to
> use css classes instead of hard-coded style properties. Search the
> patched file for "// css upgrade:" to see my comments. If my
> suggestestions seem okay, then I will create the necessary css classes
> and update the HtmlFormRenderer.java file.
>
> I know this is more than what you intended, but my thinking is to fix it
> all while we're at it.
>
> -Adrian
>
> Jacopo Cappellato wrote:
>> Adrian,
>>
>> about combining the two files into one file, for me it would be fine.
>>
>> I'd like to get a bit more feedback before applying this patch, but in
>> the meantime, if you want to test it, here is the 'correct' one
>> (attached).
>>
>> Thanks for your help.
>>
>> Jacopo