Engage: Code entry screen
colinbdclark at gmail.com
Sat Jan 30 22:04:24 UTC 2010
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:
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.
* 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. ;)
* 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:
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:
* 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,
Technical Lead, Fluid Project
More information about the fluid-work