On 06/13/2015 08:46 AM, Thomas De Schampheleire wrote:
On June 12, 2015 11:45:31 PM CEST, Mads Kiilerich <[email protected]> wrote:
On 06/11/2015 10:59 AM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire
<[email protected]>
# 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
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
