Alison,
On 15-Jul-09, at 10:38 AM, Alison Benjamin wrote:
Hi,
My first patch for FLUID-2936 made the progress unit test fail
http://build.fluidproject.org/infusion/tests/component-tests/progress/html/Progress-test.html
Ack, sorry. This is something I should have caught during code review.
Thanks for noticing.
So I have written a second patch to address this. It affects two
files. An assertion in ProgressTests.js was looking for the aria-live
property, which our first patch removed. This second patch removes
lines in ProgressTests.js that test for this. In Progress.js, the one
change I made was in the initARIA function, where I added the line
ariaElement.attr("aria-busy", "false"); .
I reviewed your code, verified that it does indeed correct the
problem, and committed it.
One tip for making your patches easier to review: trivial changes to
whitespace and line breaks can make it harder to see where substantial
changes are made. For example, if the indenting or tabs/spaces change
on a line, but the code itself doesn't change, it can be harder to read
A minor detail for sure, but it's worth keeping in mind when you're
creating your patches. If you're specifically making improvements to
indenting, whitespace, or small style details, those are very
worthwhile but are best done in a separate patch.
Nice work! This is a good improvement to the Progress component.
Colin
---
Colin Clark
Technical Lead, Fluid Project
Adaptive Technology Resource Centre, University of Toronto
http://fluidproject.org
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://fluidproject.org/mailman/listinfo/fluid-work
