Code review comment for lp:~henninge/launchpad/bug-425583-languages

Revision history for this message
Michael Nelson (michael.nelson) wrote :

> > If I click on the link to edit my preferred languages, and update them, I
> > expected to land back at this page, but it looks as though the preferred
> > languages page always redirects back to the user profile page. That should
> be
> > easy enough to fix (a next_url in the view which defaults to profile?).
>
> I did that because I don't like things like that happening to me, either. But
> it was a bit more work because the view code was not handling the redirection
> correctly in the first place.

Thanks for fixing it even though it was more work than expected!

>
> > Also, it should only be a few extra lines to call hide_and_show on a key
> press
> > event for the input box (and hide the button). Do you want to give it a go,
> or
> > did you decide against it for other reasons?
>
> I tried it out now and it actually saves a few lines of js code but it is not
> very responsive. I'd have to do the hide_and_show asynchronously but I don't
> want to put that much work into it since searching for languages is not a
> daily task anybody would do (as apposed to say adding tags to a bug).

Yes, good point. Another thought (for the future) is that we could only call hide_and_show 1 second *after* the last keypress. But as you say, it's not so important. It works really well the way you've got it.

>
> >
> > IRC Notes:
>
> I got the "unseen" class thing sorted out, it was shadowed by a more specific
> setting of display. I added an even more specific one. ;)
>
> Thanks for the review, I think this really looks good now.

np! I learned lots :)

review: Approve (ui)

« Back to merge proposal