[PATCH] comments: remove dysfunctional comment bubble on compare and file views (issue #84)

[Mads Kiilerich]

On 06/13/2015 08:46 AM, Thomas De Schampheleire wrote:
> On June 12, 2015 11:45:31 PM CEST, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 06/11/2015 10:59 AM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire
>> <thomas.de_schampheleire at alcatel-lucent.com>
>>> # Date 1432734170 -7200
>>> #      Wed May 27 15:42:50 2015 +0200
>>> # Node ID d63188974baec38503601ef0efd33a75711280a2
>>> # Parent  53d68f201e4602d3f2ccfcd27107d2ebea2deea2
>>> comments: remove dysfunctional comment bubble on compare and file
>> views (issue #84)
>>> Several compare and file views would show a comment bubble when
>> hovering code
>>> lines, even though clicking on this bubble would not do anything.
>>>
>>> Instead, make sure that the bubble only appears on pages that attach
>> a click
>>> action to the bubble (pullrequest and changeset pages), by splitting
>> the
>>> add-bubble CSS class in add-bubble (enabled when needed) and
>>> add-bubble-placeholder (always present).
>>>
>>> This change is inspired by code from Wouter Vermeiren.
>> Wouldn't it be simpler to let css (or jquery) restrict the bubble magic
>>
>> to some page types (and thus different classes of some outer diffs)?
> I'm not sure if that with simpler: it requires more nesting if the css rules, somewhat fragile when the html would be reorganized.
> Also, it requires that there actually are unique outer diffs for the relevant cases.

The nesting will however make sense ... and will be easy when we start 
using 'less'.

We already create a lot of html. The html and the DOM it creates should 
really be optimized. The jquery loop binding a click handler to all 
add-bubble elements is expensive and will (I guess?) increase the size 
of the DOM a lot. It would probably be better to give the whole table a 
class (like 'commentable') and use something like 
$('.commentable').bind('click', '.add-bubble', function...) which avoids 
modifying the DOM so much and do a bit more processing in the event 
handler. This $('.add-bubble-placeholder').addClass('add-bubble') seems 
like a pointless step in the wrong direction - it will be more efficient 
and "correct" to just add the extra class in one place in the dom (ot 
let it be there from the beginning) instead of adding it 1000 times for 
a 1000 line diff. Ok, it can be refactored together with the rest. The 
increased add-bubble-placeholder juggling might be confusing to undo.

No big deal, but it makes me feel slightly uncomfortable. What's your 
thoughts? Should we take it like this and deal with my ramblings later?

/Mads