Launchpad itself

Merge lp:~abentley/launchpad/hide-numbers into lp:launchpad

  1. hide-numbers
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/hide-numbers
Merge into: lp:launchpad
Diff against target: 114 lines
4 files modified
Makefile (+6/-1)
configs/development/launchpad-lazr.conf (+4/-0)
lib/canonical/launchpad/javascript/code/codereview.js (+34/-1)
lib/lp/code/templates/branchmergeproposal-index.pt (+10/-4)
To merge this branch: bzr merge lp:~abentley/launchpad/hide-numbers
Reviewer Review Type Date Requested Status
Curtis Hovey (community) ui Approve
Paul Hummer (community) ui Approve
Brad Crittenden (community) Approve
Review via email: mp+13982@code.launchpad.net

Commit message

Allow hiding diff line numbers.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
Partial fix for #382561: Provide the ability to show and hide line
numbers in the preview diff, using a checkbox.

Note that a fully-functional workaround for #382561 is to use the
"Download diff" link.

== Proposed fix ==

== Pre-implementation notes ==
Pre-implementation was with Thumper

== Implementation details ==
The config and Makefile changes were done so that it was possible to
generate a diff in the developer environment.

The checkbox is implemented as a Widget called NumberToggle.

== Tests ==
None

== Demo and Q/A ==
 Create a merge proposal with a diff. Click the checkbox (or the
phrase) "Show line numbers".

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/templates/branchmergeproposal-index.pt
  lib/canonical/launchpad/javascript/code/codereview.js
  Makefile
  configs/development/launchpad-lazr.conf

== JSLint notices ==
No handlers could be found for logger "bzr"
jslint: Lint found in
'/home/abentley/launchpad/hide-numbers/lib/canonical/launchpad/javascript/bugs/bugtask-index.js':
Line 584 character 31: ['api_uri'] is better written in dot notation.
        var branch_url = data['api_uri'];

^^^ I didn't change this file!

jslint: No problem found in
'/home/abentley/launchpad/hide-numbers/lib/canonical/launchpad/javascript/code/codereview.js'.

jslint: 2 files to lint.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrl+fcACgkQ0F+nu1YWqI2KtACePSXRjpjFE1lfIwppl6QLmBe2
gmoAnRNlwDgwMDWrxXsiGRCPimxEwgHp
=5eM5
-----END PGP SIGNATURE-----

Revision history for this message
Brad Crittenden (bac) wrote :

Looks good Aaron.

Please change line 8 so the comment has a period.

review: Approve
Revision history for this message
Paul Hummer (rockstar) :
review: Approve (ui)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Aaron.

I create two branches in dev, then proposed a merge. I ran make sync_branches. The toggle works well, but I an unhappy with the spacing between it and the download link. We use <ul class="horizontal"> in many places to normalise the space between user actions on a page (1.5em). I am not sure that this can be reused in the <div>. Can you experiment with the markup/CSS to make the spacing consistent with the rest of Launchpad?

review: Needs Information (ui)
Revision history for this message
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Curtis Hovey wrote:
> We use <ul class="horizontal"> in many places to normalise the
> space between user actions on a page (1.5em).
> Can you experiment with the markup/CSS to make the
> spacing consistent with the rest of Launchpad?

I've done this, and attached an incremental diff. I agree this is a bit
nicer.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrodzUACgkQ0F+nu1YWqI13qACfR6567uVNgrnaUUD40lxXMlXl
euEAn2CeMazyoVCY9PSMRe53YN57PMeQ
=BdH5
-----END PGP SIGNATURE-----

hide-numbers.diff
=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js 2009-10-26 19:16:19 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-10-28 16:49:59 +0000
@@ -144,11 +144,11 @@
144};144};
145Y.extend(NumberToggle, Y.Widget, {145Y.extend(NumberToggle, Y.Widget, {
146 renderUI: function() {146 renderUI: function() {
147 var ui = Y.Node.create('<label>' +147 var ui = Y.Node.create('<li><label>' +
148 ' <input type="checkbox" checked="checked" id="show-no"/>' +148 '<input type="checkbox" checked="checked" id="show-no"/>' +
149 '&nbsp;Show line numbers</label>');149 '&nbsp;Show line numbers</label></li>');
150 div = Y.get('#review-diff div div');150 var ul = Y.get('#review-diff div div ul.horizontal');
151 div.appendChild(ui);151 ul.appendChild(ui);
152 },152 },
153 bindUI: function(){153 bindUI: function(){
154 Y.get('#show-no').on('click', update_nos);154 Y.get('#show-no').on('click', update_nos);
155155
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-10-28 16:06:10 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-10-28 16:49:59 +0000
@@ -145,10 +145,14 @@
145 tal:define="attachment view/preview_diff/diff_text">145 tal:define="attachment view/preview_diff/diff_text">
146 <tal:real-diff condition="attachment">146 <tal:real-diff condition="attachment">
147 <div class="boardCommentDetails filename">147 <div class="boardCommentDetails filename">
148 <a tal:attributes="href attachment/getURL">148 <ul class="horizontal">
149 <img src="/@@/download"/>149 <li>
150 Download diff150 <a tal:attributes="href attachment/getURL">
151 </a>151 <img src="/@@/download"/>
152 Download diff
153 </a>
154 </li>
155 </ul>
152 </div>156 </div>
153 <div class="boardCommentBody attachmentBody">157 <div class="boardCommentBody attachmentBody">
154 <tal:diff replace="structure view/preview_diff_text/fmt:diff" />158 <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for exploring this. I too think it looks nicer.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile'
--- Makefile 2009-10-28 11:05:01 +0000
+++ Makefile 2009-10-28 16:53:12 +0000
@@ -170,6 +170,10 @@
170ftest_inplace: inplace170ftest_inplace: inplace
171 bin/test -f $(TESTFLAGS) $(TESTOPTS)171 bin/test -f $(TESTFLAGS) $(TESTOPTS)
172172
173mpcreationjobs:
174 # Handle merge proposal creations.
175 $(PY) cronscripts/mpcreationjobs.py
176
173run: inplace stop177run: inplace stop
174 $(RM) thread*.request178 $(RM) thread*.request
175 bin/run -r librarian,google-webservice -i $(LPCONFIG)179 bin/run -r librarian,google-webservice -i $(LPCONFIG)
@@ -206,7 +210,8 @@
206 # Scan branches from the filesystem into the database.210 # Scan branches from the filesystem into the database.
207 $(PY) cronscripts/branch-scanner.py211 $(PY) cronscripts/branch-scanner.py
208212
209sync_branches: pull_branches scan_branches213
214sync_branches: pull_branches scan_branches mpcreationjobs
210215
211$(BZR_VERSION_INFO):216$(BZR_VERSION_INFO):
212 scripts/update-bzr-version-info.sh217 scripts/update-bzr-version-info.sh
213218
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf 2009-10-20 11:29:26 +0000
+++ configs/development/launchpad-lazr.conf 2009-10-28 16:53:12 +0000
@@ -182,6 +182,10 @@
182soft_max_size: 40000182soft_max_size: 40000
183hard_max_size: 1000000183hard_max_size: 1000000
184184
185[mpcreationjobs]
186error_dir: /var/tmp/codehosting.test
187oops_prefix: DMPCR
188
185[personalpackagearchive]189[personalpackagearchive]
186root: /var/tmp/ppa/190root: /var/tmp/ppa/
187private_root: /var/tmp/ppa191private_root: /var/tmp/ppa
188192
=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js 2009-08-03 20:11:00 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-10-28 16:53:12 +0000
@@ -124,4 +124,37 @@
124}124}
125125
126126
127}, '0.1', {requires: ['base', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});127var NumberToggle = function () {
128 NumberToggle.superclass.constructor.apply(this, arguments);
129};
130
131
132var update_nos = function(){
133 var new_display = 'none';
134 if (this.get('checked')) {
135 new_display = 'block';
136 }
137 Y.all('td.line-no').setStyle('display', new_display);
138};
139
140
141NumberToggle.NAME = 'numbertoggle';
142
143NumberToggle.ATTRS = {
144};
145Y.extend(NumberToggle, Y.Widget, {
146 renderUI: function() {
147 var ui = Y.Node.create('<li><label>' +
148 '<input type="checkbox" checked="checked" id="show-no"/>' +
149 '&nbsp;Show line numbers</label></li>');
150 var ul = Y.get('#review-diff div div ul.horizontal');
151 ul.appendChild(ui);
152 },
153 bindUI: function(){
154 Y.get('#show-no').on('click', update_nos);
155 }
156});
157
158Y.codereview.NumberToggle = NumberToggle;
159
160}, '0.1', {requires: ['base', 'widget', 'lazr.anim', 'lazr.formoverlay', 'lp.picker']});
128161
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-10-23 02:41:08 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-10-28 16:53:12 +0000
@@ -145,10 +145,14 @@
145 tal:define="attachment view/preview_diff/diff_text">145 tal:define="attachment view/preview_diff/diff_text">
146 <tal:real-diff condition="attachment">146 <tal:real-diff condition="attachment">
147 <div class="boardCommentDetails filename">147 <div class="boardCommentDetails filename">
148 <a tal:attributes="href attachment/getURL">148 <ul class="horizontal">
149 <img src="/@@/download"/>149 <li>
150 Download diff150 <a tal:attributes="href attachment/getURL">
151 </a>151 <img src="/@@/download"/>
152 Download diff
153 </a>
154 </li>
155 </ul>
152 </div>156 </div>
153 <div class="boardCommentBody attachmentBody">157 <div class="boardCommentBody attachmentBody">
154 <tal:diff replace="structure view/preview_diff_text/fmt:diff" />158 <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
@@ -181,6 +185,8 @@
181 Y.code.codereview.connect_links();185 Y.code.codereview.connect_links();
182 });186 });
183187
188 (new Y.codereview.NumberToggle()).render();
189
184 });190 });
185 -->191 -->
186<tal:script replace="structure string:&lt;/script&gt;" />192<tal:script replace="structure string:&lt;/script&gt;" />