Hi Sveto, This is really awesome stuff! Some code review comments below. I think we're pretty much ready to get this into trunk and link it up to the home screen.
On 2010-01-29, at 11:23 AM, Svetoslav Nedkov wrote: > The object code entry screen > for Engage is near ready. > > You can check out the code at: > > https://source.fluidproject.org/svn/scratchpad/objectCodeEntry Really nice work. I've divided up my review notes into three sections: Before Merge, Before Release, and Down the Road. I'd be happy to merge your changes into trunk once you've had a chance to do the first set of tasks, and then we can all share the work that needs to get done before release. Before Merge ------------------ * Use the fluid.engage namespace for your code. We've recently been tidying this up in all of our components. * Why do you use fluid.invokeGlobalFunction() in your init block rather than just calling fluid.codeEntry()? * The number pad images are a bit pixellated. I've attached PNGs of the number pad sourced directly from the wirefreames to this JIRA: http://issues.fluidproject.org/browse/ENGAGE-305 * I think the user experience would be improved if we wait a brief moment before redirecting to the ArtifactView page. At the moment, the user can't see the second digit before the page starts to reload. James, what do you think? * Is there a reason why you're making a synchronous Ajax request in checkCode()?Your code looks like it's built to support asynchronicity (which is great), so why not use it? * There's a funny block of duplicate code in your setup() function. Create a selector that matches all buttons in DOM order, and then this will be the sort of job that the for loop or each() function were made for. ;) Before Release --------------------- * We probably have some accessibility work to do on this page. I did some quick testing with the iPhone Accessibility Inspector, but there's more testing to be done. Off the top of my head, I think we should: - Make each number button a real button, or, if all else fails, give each number an ARIA button role - Improve the code display area--it might make sense as HTML input elements, or perhaps a live region? The core problem at the moment is that a non-sigted user will be unaware of the code display area when no digits have been entered. * I know that our URL space is pretty weedy at the moment, but let's try to tidy it up a bit by mounting the service and the template within the artifacts/ resource like this: http://server.org/artifacts/objectCode.html/.json Down the Road --------------------- * We should use real HTML buttons and CSS styling instead of images for the number pad * I think event delegation might make sense for the number pad--attaching a single handler to the container of all the buttons and then determining the action based on the target of the event. * We might consider giving this component a real public API. Something along the lines of: deleteLastDigit() enterCode(fullCode) enterDigit(digit) getArtifactURLForCode(fullCode) * It occurs to me that it might make sense to actually include Object Code Entry as a subcomponent of ArtifactView. That way, we can actually move from one screen to another with the minimum of extra server round trips and better responsiveness. I hope this helps, Colin --- Colin Clark Technical Lead, Fluid Project http://fluidproject.org _______________________________________________________ fluid-work mailing list - [email protected] To unsubscribe, change settings or access archives, see http://fluidproject.org/mailman/listinfo/fluid-work
