[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#761103: debsources: highlight #Lxxx lines by default

Hello again, Jason.

To JavaScript hackers lurking here: your comments on this patch are most

On Sat, Nov 01, 2014 at 11:37:56AM -0400, Jason Pleau wrote:
> I have attached a patch that solves this bug.

Thanks a lot for this patch. It works like a charm on my local
deployment. As I'm no JavaScript guru, I could use extra eyeballs to
review it, and I do have some questions for you (see below), but I see
no reason why this couldn't be integrated.

> -        <a id="L{{ i }}" href="#L{{ i }}">{{ i }}</a><br />
> +        <a id="L{{ i }}" href="#L{{ i }}" class="linenumber" data-line="{{ i }}">{{ i }}</a><br />

Is it really needed to add class="linenumber" to all <a> elements?

It seems that the JavaScript only uses that class as a selector. So
can't one instead rely on the id="sourcelinenumbers" that is already on
the parent <pre> element, and consider all its <a> children? That would
avoid bloat in the generated HTML, reduce load time, etc.

> +<script type="text/javascript">
> +</script>

I'd like to move this JavaScript snippet to a separate .js file, rather
than re-shipping it every time a file is rendered. Can you change your
patch to do so?

I suggest to name it something like
DEBSOURCES_ROOT/debsources/app/static/javascript/debsources.js (so that
we can use it in the future for other JavaScript-related needs). It will
then be accessible from the HTML as "/static/javascript/debsources.js"
(note: *not* as /javascript/debsources.js).

Stefano Zacchiroli  . . . . . . .  zack@upsilon.cc . . . . o . . . o . o
Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o
Former Debian Project Leader  . . @zack on identi.ca . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »

Attachment: signature.asc
Description: Digital signature

Reply to: