Object Entry Code Review

[Sambhavi Chandrashekar]

An issue I found while trying out object code entry on iPhone (both as an
app and through Safari without VoiceOver and with VoiceOver) is that I am
not transferred automatically to the artifact screen upon entering the
second digit. Whereas with Safari on Mac, upon entering the second digit I
get a message 'Opening artifact page' and the page opens up.

Sam

On Fri, Feb 12, 2010 at 12:33 PM, Justin Obara <obara.justin at gmail.com>wrote:

> Hi Sveto,
>
> So I've committed your latest patch. I've assigned it to colin to take
> another look at it though so he may have some more comments coming.
>
> I noticed that you worked in some of the changes that I had mentioned,
> that's great.
>
> One more thing to add though is for the unit  tests, it looks like you
> could do some refactoring to get rid of some of the repeated code. This is a
> mistake that I often make with unit tests.
>
> Thanks
> Justin
>
> On 2010-02-11, at 8:19 PM, Justin Obara wrote:
>
> > 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.
>
> _______________________________________________________
> fluid-work mailing list - fluid-work at fluidproject.org
> To unsubscribe, change settings or access archives,
> see http://fluidproject.org/mailman/listinfo/fluid-work
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://fluidproject.org/pipermail/fluid-work/attachments/20100212/87dd6fbc/attachment.html>