Code review comment for lp:~thumper/launchpad/use-last-rev-id

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

> Need to find a good way to test this.

Heh, a script to setup the environment that you demo'd would be great. I just created an MP for ~name12/gnome-terminal/scanned but that of course didn't have a last revision, but I could play a little with it.

UI-wise, it works really well, updating the display with the extra row when necessary. I laughed in your video near the end where you go to update the version number, but then hesitate and stop the video... made me click it ;). So I assume that'll get done later... do you think it's worth doing it before landing this branch? The non-js fallback works fine, but it does seem strange that it doesn't bring up an overlay and update inline. Up to you.

Now, nothing to do with a UI review, but I wrote a DynamicDomUpdater a while back for this *type* of thing - basically it allows you to plug-in the functionality so that a node (for example, a table node) knows how to update itself. YMMV, but you can find it at:

lib/canonical/launchpad/javascript/soyuz/lp_dynamic_dom_updater.js

tested at:

lib/canonical/launchpad/javascript/soyuz/tests/lp_dynamic_dom_updater.js

Cheers,
Michael.

review: Approve (ui)

« Back to merge proposal