Hi Colin,
Thank you for you quick reply. I have made the first set of changes and
committed them to scratchpad.
I have written some comments inline:
Colin Clark wrote:
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
We are now using the new images, one thing I wondered about was that
there was a transparent margin around each of them, so the obvious way
to lay them out was to put a negative margin in the css to compensate
those gaps. I reckon that this could be totally wrong.
* 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?
I have added a delay of one second in the component options.
* 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 now have a fancy selector replacing this code and -20 lines. :)
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.
About the code display area Justin mentioned that we don't want the
iPhone native keyboard to pop-up when on the code entry screen.
I will do more tests and will make sure that those problems are resolved.
* 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
