Re: Please review the attached patch fro the HtmlFormRenderer class

Posted by Adrian Crum on Jan 09, 2007; 3:58pm
URL: http://ofbiz.116.s1.nabble.com/Please-review-the-attached-patch-fro-the-HtmlFormRenderer-class-tp176227p176236.html

Jacopo,

Thanks for taking a look at this! Comments inline:

Jacopo Cappellato wrote:
> 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.

I'm not doing any reformatting manually - my editor must be doing that without
my knowledge. I don't know how to make it stop. I apologize for any confusion it
causes.

One of the things I noticed about this java file is that it doesn't follow the
current OFBiz formatting guidelines. I would like to reformat the entire file to
bring it up to standard. What do you think?

> 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\"");
> }

That's exactly the approach I had in mind!

I'll work on this today and get it submitted.