2010/2/6 Jakub Narebski <jnareb@xxxxxxxxx>:
> On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
>
>> Subject: [PATCH 2/4] gitweb: show notes in shortlog view
>
> Is it RFC?
See reply to comments on 1/4 8-/
> Why it is only for 'shortlog' view, and not also for 'history' which is
> also shortlog-like view? Or is there reason why it is not present for
> 'history' view?
I always forget about history view, probably because I never use it.
>> The presence of the note is shown by a small icon, hovering on which
>> reveals the actual note content.
>
> Signoff?
A-hem. (whistles innocently)
>> +
>> +span.notes span.note-container:before {
>> + content: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAgAAAAIAgMAAAC5YVYYAAAAAXNSR0IArs4c6QAAAAlQTFRFAABnybuD//+t5rXizQAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAAGElEQVQI12MIDWXIWglDQHYIQ1YAQ6gDAFWPBrAKFe0fAAAAAElFTkSuQmCC');
>> +}
>
> Not all web browsers support ':before' pseudo-element, and 'content'
> (pseudo-)property.
I know it's neither good form nor good webdesigner attitude, but I
stopped caring about IE a long time ago. I understand however that
some ancient versions of Mozilla browsers might have the same issue
too.
> Not all web browsers support 'data:' URI schema in CSS; also such image
> cannot be cached (on the other hand it doesn't require extra TCP
> connection on first access, and CSS file is cached anyway).
>
> On the other hand adding extra images to gitweb would probably require
> additional (yet another) build time parameter to tell where static
> images are (besides logo and favicon).
>
> So perhaps it is good solution, at least for a first attempt.
A possible alternative could maybe do without images and just use
borders and backgrounds of an 8x8 fixed-size element. Wouldn't look as
nice, probably, but should render decently in everything that supports
CSS1.
>> +# display notes next to a commit
>> +sub format_notes_html {
>> + my %notes = %{$_[0]};
>
> Why not use 'my $notes = shift;', and later '%$notes'?
No particular reason. I didn't check for syntax preferences regarding
this in gitweb, or I would have noticed there was a preference for the
one you mention.
>> + my $ret = "";
>
> Perhaps $return or $result would be a better name, to better distinguish
> it from visually similar $ref (see $ref vs $res);
Yep, good point.
>> + while (my ($ref, $text) = each %notes) {
>> + # remove 'refs/notes/' and an optional final s
>> + $ref =~ s/^refs\/notes\///;
>
> You can use different delimiter than / to avoid 'leaning toothpick'
> syndrome, e.g.: $ref =~ s!^refs/notes/!!;
Indeed I should.
>> + $ref =~ s/s$//;
>> +
>> + # double markup is needed to allow pure CSS cross-browser 'popup'
>> + # of the note
>> + $ret .= "<span title='$ref' class='note-container $ref'>";
>> + $ret .= "<span title='$ref' class='note $ref'>";
>> + foreach my $line (split /\n/, $text) {
>> + $ret .= esc_html($line) . "<br/>";
>
> Probably would want
>
> $ret .= esc_html($line) . "<br/>\n";
>
> here. Or do we want single string here?
It's within a span element so I was trying to stick to single line in
the HTML source.
> Also, do you want/need final <br>? If not, perhaps
>
> join("<br/>", map { esc_html($_) } split(/\n/, $text);
>
> would be a better solution (you can always add final "<br/>" later)?
I did notice that the final br didn't seem to affect the box height,
so I didn't bother looking at ways to do without it, but it's probably
nicer to not have it.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html