launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

A couple of weird behavioural bugs that I've noticed:

 - Posting a comment doesn't JSify the "View inline comments" link. One must refresh the page.

 - Clicking the "View inline comments" link will refresh a diff even if it was already shown. This means you end up scrolling down to a spinner, the inline comments disappear for a second, and then reappear to look identical to the state when the scroll occurred. As I mention below, a saner navigator API might help avoid this unnecessary refresh.

And now some comments on specific bits of the code... I really wish those LP devs would decide that inline comments were a useful feature. They're so out of touch.

7	@cachedproperty
8	+ def previewdiff_id(self):

This will execute a couple of queries for each comment shown on an MP page. You can avoid one of them by using 'return inline_comment.previewdiff_id', but the other you can't prevent without a bulk query to prepopulate the cachedproperty.

167	+ Y.all('.inline-comment-scroller').each( function(node) {
168	+ rc.link_scroller(node, '#review-diff', function() {
169	+ var previewdiff_id = node.one('span').get('text');
170	+ navigator.set('value', previewdiff_id);
171	+ navigator.simulate('change');
172	+ });
173	+ })

This will work, but there must be a nicer way in YUI to have the scroller component call a method on the navigator, rather than doing it all manually.

202	+ <span style="float: right;" tal:condition="context/previewdiff_id">
203	+ <a class="inline-comment-scroller" href="">
204	+ View inline comments
205	+ <span style="display: none;" tal:content="context/previewdiff_id"/>
206	+ </a>
207	+ </span>

Did you consider using a data attribute on the comment div? Something like tal:attributes="data-previewdiff-id context/previewdiff_id", then use div.getAttribute("data-previewdiff-id") in JS. It avoids the extra invisible element.
-- 
https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/213950
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.