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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
