| Submitter | Alexander Plavin |
|---|---|
| Date | July 12, 2013, 12:07 p.m. |
| Message ID | <161cf6af00a4187cf2eb.1373630872@debian-alexander.dolgopa> |
| Download | mbox | patch |
| Permalink | /patch/1841/ |
| State | Superseded, archived |
| Headers | show |
Comments
Alexander Plavin <me@aplavin.ru> writes: > # HG changeset patch > # User Alexander Plavin <me@aplavin.ru> > # Date 1373630293 -14400 > # Fri Jul 12 15:58:13 2013 +0400 > # Node ID 161cf6af00a4187cf2ebc19fe95f1b990de84133 > # Parent 6187d6255fd2a14d7d21e27cd0d86ce6f282a86c > hgweb: add line wrapping switch with javascript > > diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/paper/filerevision.tmpl > --- a/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 16:01:11 2013 +0400 > +++ b/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 15:58:13 2013 +0400 > @@ -67,6 +67,7 @@ > </table> > > <div class="overflow"> > +<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div> > <div class="sourcefirst"> line source</div> > <pre class="sourcelines">{text%fileline}</pre> > <div class="sourcelast"></div> > diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/static/mercurial.js > --- a/mercurial/templates/static/mercurial.js Fri Jul 12 16:01:11 2013 +0400 > +++ b/mercurial/templates/static/mercurial.js Fri Jul 12 15:58:13 2013 +0400 > @@ -269,3 +269,16 @@ > document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none'; > document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline'; > } > + > +function setLinewrap(flag) { > + var nodes = document.querySelectorAll('.sourcelines > span'); > + for (var i = 0; i < nodes.length; i++) { > + nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre'; > + } Would it not be better to set a class (such as "wrap") on the pre element and then add pre.wrap span { white-spare: pre-wrap; } to the stylesheet? That way you don't need to run through a potentially huge number of lines and we can collect the style information in the stylesheet. > + var links = document.getElementsByClassName('linewraplink'); I would prefer something like var not_flag = flag == 'on' ? 'off' : 'on'; > + for (var i = 0; i < links.length; i++) { links[i].innerHTML = flag; links[i].href = 'javascript:setLinewrap("' + not_flag + '");' > + } > +} Alternatively, and probably better: avoid the flag parameter and let the DOM keep the state instead. That is, if the "wrap" class is there, then remove it and update the innerHTML appropriatedly -- there is then no need to update the link since toggleLinewrap (new name for the function) would work without any arguments.
2013/7/12 Martin Geisler <martin@geisler.net>: > Alexander Plavin <me@aplavin.ru> writes: > >> # HG changeset patch >> # User Alexander Plavin <me@aplavin.ru> >> # Date 1373630293 -14400 >> # Fri Jul 12 15:58:13 2013 +0400 >> # Node ID 161cf6af00a4187cf2ebc19fe95f1b990de84133 >> # Parent 6187d6255fd2a14d7d21e27cd0d86ce6f282a86c >> hgweb: add line wrapping switch with javascript >> >> diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/paper/filerevision.tmpl >> --- a/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 16:01:11 2013 +0400 >> +++ b/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 15:58:13 2013 +0400 >> @@ -67,6 +67,7 @@ >> </table> >> >> <div class="overflow"> >> +<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div> >> <div class="sourcefirst"> line source</div> >> <pre class="sourcelines">{text%fileline}</pre> >> <div class="sourcelast"></div> >> diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/static/mercurial.js >> --- a/mercurial/templates/static/mercurial.js Fri Jul 12 16:01:11 2013 +0400 >> +++ b/mercurial/templates/static/mercurial.js Fri Jul 12 15:58:13 2013 +0400 >> @@ -269,3 +269,16 @@ >> document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none'; >> document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline'; >> } >> + >> +function setLinewrap(flag) { >> + var nodes = document.querySelectorAll('.sourcelines > span'); >> + for (var i = 0; i < nodes.length; i++) { >> + nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre'; >> + } > > Would it not be better to set a class (such as "wrap") on the pre > element and then add > > pre.wrap span { > white-spare: pre-wrap; > } > > to the stylesheet? That way you don't need to run through a potentially > huge number of lines and we can collect the style information in the > stylesheet. Agree, changed this. > >> + var links = document.getElementsByClassName('linewraplink'); > > I would prefer something like > > var not_flag = flag == 'on' ? 'off' : 'on'; >> + for (var i = 0; i < links.length; i++) { > links[i].innerHTML = flag; > links[i].href = 'javascript:setLinewrap("' + not_flag + '");' >> + } >> +} > > Alternatively, and probably better: avoid the flag parameter and let the > DOM keep the state instead. That is, if the "wrap" class is there, then > remove it and update the innerHTML appropriatedly -- there is then no > need to update the link since toggleLinewrap (new name for the function) > would work without any arguments. Yes, this really looks better :) Thanks for your comments here. Sent a new version considering these suggestions. > > -- > Martin Geisler
Alexander Plavin <me@aplavin.ru> writes: > 2013/7/12 Martin Geisler <martin@geisler.net>: >> Alexander Plavin <me@aplavin.ru> writes: >> >>> + var links = document.getElementsByClassName('linewraplink'); >> >> I would prefer something like >> >> var not_flag = flag == 'on' ? 'off' : 'on'; >>> + for (var i = 0; i < links.length; i++) { >> links[i].innerHTML = flag; >> links[i].href = 'javascript:setLinewrap("' + not_flag + '");' >>> + } >>> +} >> >> Alternatively, and probably better: avoid the flag parameter and let >> the DOM keep the state instead. That is, if the "wrap" class is >> there, then remove it and update the innerHTML appropriatedly -- >> there is then no need to update the link since toggleLinewrap (new >> name for the function) would work without any arguments. > > Yes, this really looks better :) > > Thanks for your comments here. Sent a new version considering these > suggestions. I was just thinking out loud, so I'm glad you liked the suggestions and thanks for turning them into a proper patch :)
Patch
diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/paper/filerevision.tmpl --- a/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 16:01:11 2013 +0400 +++ b/mercurial/templates/paper/filerevision.tmpl Fri Jul 12 15:58:13 2013 +0400 @@ -67,6 +67,7 @@ </table> <div class="overflow"> +<div class="sourcefirst" style="float: right;">line wrap: <a class="linewraplink" href="javascript:setLinewrap('off')">on</a></div> <div class="sourcefirst"> line source</div> <pre class="sourcelines">{text%fileline}</pre> <div class="sourcelast"></div> diff -r 6187d6255fd2 -r 161cf6af00a4 mercurial/templates/static/mercurial.js --- a/mercurial/templates/static/mercurial.js Fri Jul 12 16:01:11 2013 +0400 +++ b/mercurial/templates/static/mercurial.js Fri Jul 12 15:58:13 2013 +0400 @@ -269,3 +269,16 @@ document.getElementById('diffstatdetails').style.display = flag ? 'inline' : 'none'; document.getElementById('diffstatexpand').style.display = flag ? 'none' : 'inline'; } + +function setLinewrap(flag) { + var nodes = document.querySelectorAll('.sourcelines > span'); + for (var i = 0; i < nodes.length; i++) { + nodes[i].style.whiteSpace = flag == 'on' ? 'pre-wrap' : 'pre'; + } + + var links = document.getElementsByClassName('linewraplink'); + for (var i = 0; i < links.length; i++) { + links[i].innerHTML = flag == 'on' ? 'on' : 'off'; + links[i].href = 'javascript:setLinewrap("' + (flag == 'on' ? 'off' : 'on') + '");' + } +}