Object Entry Code Review

[Justin Obara]

Hi Sveto,

I'll try to take a look at your patch today if I can. We are supposed to be moving on to testing today, but if we don't I'll see if I can get it applied.

Also for screen readers, you could try the following:

Linux:

ORCA -> http://live.gnome.org/Orca

Windows:

NVDA -> http://www.nvda-project.org/

Thanks
Justin


On 2010-02-15, at 8:30 AM, Svetoslav Nedkov wrote:

> Hi Justin, Sam,
> 
> I've attached a patch to http://issues.fluidproject.org/browse/ENGAGE-379 that adds the wrong code icon.
> 
> Sam, I've also added a aria-label attribute to the code entry fields, could you recommend a screen reader for Linux or Windows that recognizes ARIA attributes? Does the problem with redirection with iPhone persist?
> 
> 
> Regards,
> 
> Svetoslav
> 
> 
> Sambhavi Chandrashekar wrote:
>> 
>> 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/20100216/75c71a26/attachment.html>