Code review comment for lp:~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave

On Thu, Dec 03, 2009 at 11:22:46AM -0000, Michael Nelson wrote:
> Review: Needs Information ui
> > This branch does the final connecting up of JS to make the
> > Asynchronous Dupefinder stuff work.
>
> This is great! I've got three points below that I think are worth
> addressing - see what you think.
>
> >
> > How to test this branch (UI):
> >
> > 1. Go to launchpad.dev/firefox/+filebug 2. Enter a simple title
> > (say, 'a') and hit "next" 3. You should get a list of possible
> > duplicates for that bug. 4. You should be able to interact with
> > those dupes in the same way as you always do (subscribe, mark as
> > affecting you, etc). 5. You should also be able to file a new bug
> > if you need to in the same way as you do currently.
> >
> > Issues that I'm aware of at this stage:
> >
> > 1. The animations seem to happen at the wrong time, so slide_ins
> > get played after the element has already been rendered as closed.
> > I'm looking into this.
>
> OK - at first I thought this was a feature :-) (as I try not to read
> the MP before trying out the feature).
>

I've tried to fix this by making the animation duration as short as
possible but that doesn't work terribly well. I think that I can fix it
using some event jiggery-pokery, and I'm still working on that (see next
comment).

>
> > 2. The way that the dupes appear in the page isn't exactly pretty.
> > I'm open to suggestions about how this should work.
>
> It would be nice to see them slide in... once you have the results,
> add them to the dom, but hidden, and then slide out the div containing
> them all?

I'm still working on getting this right. Please feel free to read the
rest of my response and I'll get back to you about this and the above
item.

> My first thought is that the phrase: 'Is "a" one of these bugs?' seems
> strange. Even with a proper summary, 'Is "A bunch of soyuz source
> pages are missing headings" one of these bugs?' seems strange. Do you
> know the history of that decision? My guess would be that, since the
> summary entered by the user *wasn't* displayed anywhere else once they
> had searched, it needed to be included on the page somewhere. But now
> with your changes here, that the summary and input box remain on the
> page (at least for the JS version), I think we could just say
> something like 'Do any of the following bugs describe the bug you are
> trying to report?'. What do you think? It's great that they can now
> still see their search and update it if desired!

Right, I agree. That text is a hold-over, so I've updated the view and
the templates to make it a bit more meaningful in an inline dupe list.
It'll remain the same should someone use the non-ajax version of the
page, though.

> Secondly, currently in your branch it seems that you cannot have the
> 'No, I need to report a new bug' section and one of the other
> potential duplicates open at the same time. For example, as a person
> I:
>
> 1. enter a summary and hit Next, 2. briefly read the titles, but
> nothing rings a bell, so I click 'No, I need to report a new bug", 3.
> I start adding the further information, and then realise, hey I want
> to check one of those bugs above, as it actually could be the same, 4.
> I click on the expander and zap, my new bug description disappears.
>
> Of course, the info I entered is still there, and I'll see it again if
> I click again on "No, I need to report a new bug", but I don't see any
> reason for it disappearing in the first place? (with less
> justification, the same goes in reverse, closing the expanded dups
> when I click on 'No, I need to report a new bug.').

You're right. That's part of the current design which has always been a
bit contentious. I've fixed it so that neither of these should happen
any more.

>
> Third, just an implementation detail, but hitting Enter while the
> focus in the summary field doesn't filter - it re-loads the page,
> which I'm assuming is not what you want.

Nope. I've fixed this to use Y.on('key'... 'down:13'...) so that hitting
enter in the search box will cause the search to be re-run.

> Thanks for resurrecting this work! It'll be a great improvement :)

Thanks! FYI, I'll be requesting a code and JS review for it shortly.

Also, I've added some error handling to the JS, so that if something
goes wrong when searching for dupes the user will still be able to file
a bug. You can demo this by applying the patch at [1] to the tree and
re-running Launchpad.

 [1] http://pastebin.ubuntu.com/337160/