Gerrit Goetsch [2004-11-12 11:31 UTC] Looks nice, but I prefer "HTML_QuickForm_Selectfilter" as class name, not "HTML_QuickForm_selectfilter".
In according to the CS, please remove the tabstops and replace them with spaces.
Nicolas Hoizey [2004-11-12 11:58 UTC] The class name uses the same convention as other QF elements. Maybe I should use the same also for the package name...
I have removed the tabs.
Justin Patrin [2004-11-12 18:47 UTC] There should be spaces around the '=' in the constructor declaration.
I personally don't like heredocs. I'd rather you use ' for the string and change str == '' to str == "" within the JS.
You have:
this.form[\''.$target.'\']
would this be cleaner?
this.form["'.$target.'"]
As long as it *works*, of course, but any quotes should work.
Again here:
"<script type=\"text/javascript\">\n//<![CDATA[\n"
could be:
"<script type='text/javascript'>\n//<![CDATA[\n"
or:
'<script type="text/javascript">
//<![CDATA[
'
I prefer HTML_QuickForm_SelectFilter as the class name. Yes, it's different from the other QF elements, but it conforms to PEAR CS correctly. QF shouldn't have a problem with this class name as it doesn't do any caps changing on the class name or include filename.
Nicolas Hoizey [2004-11-15 09:46 UTC] I've just followed coding style I found in other QF elements.
Even if I share your point of vue, I would like to read what QF maintainers think before modifying anything.
Nicolas Hoizey [2004-11-17 09:03 UTC] The new version now follows as strictly as possible the CS.
The examples now use a countries list, which gives a better impact.
|
