Tuesday, Jun 29, 1:00PM
Jun. 29th, 2010 01:04 pmBug counts: resolved 198, assigned 11, needs-review 8
I spent some time today helping
My main issue with the initial patch was that it needed better grouping of similar things. That is to say, if the code looks like this:
do thing1 if can_do_it; do thing2 if can_do_it; do thing3 if can_do_it;
It's easier to read and maintain if we reformat it like so:
if (can_do_it) {
do thing1;
do thing2;
do thing3;
}I also said "OMG do we really need all those js files?"
I briefly went down a wrong path when I thought we only needed to load the button from ReplyPage.pm, but I later realized we needed to account for sitescheme comment pages as well, which use talkpost.bml and talkpost_do.bml. And we need to load the js files on each of those three different pages that call this function. Double sigh.
There was one last bug in the final submission that
This will definitely be a target for later refactoring, very similar to what I did for the quote button in bug 2693. Sometimes I have to remind myself not to jump on it right away. Get it working first; refactor it later. This one is more annoying than the quote button, though, because the quote button wasn't loading js libraries...