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

[Thomas De Schampheleire]

On Wed, Jun 17, 2015 at 9:39 PM, Thomas De Schampheleire
<patrickdepinguin at gmail.com> wrote:
> On Wed, Jun 17, 2015 at 1:23 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> 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.
>
> I'm not sure I fully understand this idea: giving the table a class
> .commentable and binding a click function to it means that clicking
> anywhere inside the table would fire the click handler. So when you
> say 'a bit more processing in the event handler' you mean to determine
> which object actually got clicked and return immediately if it's
> anything other than the bubble ?

I looked deeper into this: I think you in any case need to have a
.add-bubble class on every table row, because hovering it should make
it visible. One could have javascript checking any hover action on the
full table and show the bubble when hovering an appropriate element,
but I think this will become pretty complex, if at all possible.
Currently the displaying-on-hover is completely handled with CSS in
about three lines.

>
>
>> 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?
>
> No, I don't think there is a need to go too fast here. It's not an
> urgent issue and so far I've been the only one to complain about it
> :-o

I just sent another approach, using the surrounding commentable-diff
idea. The patch turns out to be very small, indeed much better than
the first approach.

Let me know what you think...

/Thomas