Object Entry Code Review

Justin Obara obara.justin at gmail.com
Fri Feb 12 01:19:31 UTC 2010


Hey Sveto,

So I've taken  a look at your Object Entry Code patch and general code. I have committed it into the repository with some changes.

Please see below for some thoughts. Hope it all makes sense.

Thanks
Justin

Here are a few comments.
------------------------------------

1) You should always try to run jslint (http://www.jslint.com/) before committing/patching. There was a bunch of changes that I needed to make because of this, mostly due to tabs instead of spaces (we use 4 spaces) and probably other things that were caused by eclipse's auto-formatting.

2) We have a convention for our selectors. All css (styling related selectors) begin with "fl-", all DOM selectors begin with "flc-" if you need to both find and style an element you should use two class names. Also here is the format we use "flc-componentName-thing" and "fl-componentName-thing"

Some changes I made.
---------------------------------

You should probably take a look at the current code to see all the changes, but I'll mention a couple here.

1) Internationalized the alt text for the delete button

2) Disabled the delete button while checking the validity of the code

3) Changed the selector for the buttons, to have one for all the digits and another one for the delete button

4) Removed some js files that were included but never used

Things that still need to be done. 
--------------------------------------------

(Note that these don't necessarily have to be done for 0.3b)

1) The atDigit logic seems a bit complex/confusing there may be a better way to do this.

2) The service should be converted to use our new "spout" method. You can see a demonstration of this in Colin's refactoring of My Collection

3) The unit tests are broken. (note: i wasn't able to apply your second patch because i had already made too many changes, but did remove the test code manually. A new patch for the unit tests may bring them back to life).

4) In the service you are returning whether or not an artifact was found. Instead you should only return the necessary data (the URL) on success and return an appropriate error message on failure to find an artifact. The client side code should receive this error and act upon it. That way for your ajax call, success would be the transition to the correct artifact page, and the error would be the invalid code message and resetting of the necessary parts.


More information about the fluid-work mailing list