pagure

Clone

#1883 Show icon to jump to changed file from diff
Merged 7 years ago by pingou. Opened 7 years ago by cep.
cep/pagure feature_jump_to_file_from_diff  into  master

file modified
+18 
@@ -43,6 +43,9 @@ 
 

  .code_table tr td pre{
 

      padding: 0;
 

      margin: 0;
 

+     /* prevent scrollbars appearing on each line when
 

+     zooming in some cases */
 

+     overflow: hidden;
 

  }
 

  

  .code_table tr .cell1{
 
@@ -67,6 +70,21 @@ 
 

      padding-right:3px;
 

  }
 

  

+ .open_changed_file_icon_wrap {
 

+   /* Hidden until user hovers over it */
 

+   visibility: hidden;
 

+ }
 

+ 

+ .open_changed_file_icon {
 

+   /* some adjustments to make the icon look better */
 

+   font-size: 13px !important; /* default size turns out too small here */
 

+   top: 3px !important; /* work around a small vertical misalignment */
 

+ }
 

+ 

+ .open_changed_file_icon:hover {
 

+   cursor: pointer;
 

+ }
 

+ 

  .alert-info pre
 

  {
 

      background-color:#CFE5F0;

file modified
+10 
@@ -1393,6 +1393,16 @@ 
 

  $(document).on('click', '#pr-tabs a', function() {
 

    window.location.hash = $(this).attr('href');
 

  });
 

+ 

+ // Show an icon to open the changed file, when the user hovers over the
 

+ // @@ -x,y +x,y @@ line in the diff. Clicking this icon opens the file (at the
 

+ // relevant line number) in a new tab.
 

+ $(document).on("mouseenter", "td.cell2", function(){
 

+ 	$(this).find("a.open_changed_file_icon_wrap").css('visibility', 'visible');
 

+ });
 

+ $(document).on("mouseleave", "td.cell2", function() {
 

+   $(this).find("a.open_changed_file_icon_wrap").css('visibility', 'hidden');
 

+ });
 

  </script>

file modified
+20 
@@ -140,6 +140,26 @@ 
 

              continue
 

          if line.startswith('<div'):
 

              line = line.split('<pre style="line-height: 125%">')[1]
 

+             if prequest:
 

+                 rangeline = line.partition('font-weight: bold">@@ ')[2] \
 

+                     if line.partition('font-weight: bold">@@ ')[1] == \
 

+                     'font-weight: bold">@@ ' else None
 

+                 if rangeline:
 

+                     rangeline = rangeline.split(' @@</span>')[0]
 

+                     linenumber = rangeline.split('+')[1].split(',')[0]
 

+                     line = line + '&nbsp;<a href="%s#_%s" target="_blank" ' % (
 

+                         flask.url_for(
 

+                             'view_file',
 

+                             repo=prequest.project_from.name,
 

+                             username=prequest.project_from.user.username
 

+                             if prequest.project_from.is_fork else None,
 

+                             namespace=prequest.project_from.namespace,
 

+                             identifier=prequest.branch_from,
 

+                             filename=filename), linenumber)
 

+                     line = line + 'class="open_changed_file_icon_wrap">' + \
 

+                         '<span class="oi open_changed_file_icon" ' + \
 

+                         'data-glyph="eye" alt="Open changed file" ' + \
 

+                         'title="Open changed file"></span></a>'
 

          output.append('<td class="cell2"><pre>%s</pre></td>' % line)
 

          output.append('</tr>')

file modified
+13 
@@ -254,6 +254,13 @@ 
 

          self.assertIn(
 

              'title="View file as of 2a552b">sources</a>', output.data)
 

  

+         # Test if the `open changed file icon` is displayed.
 

+         self.assertIn(
 

+             'class="open_changed_file_icon_wrap"><span '
 

+             'class="oi open_changed_file_icon" data-glyph="eye" '
 

+             'alt="Open changed file" title="Open changed file"></span>'
 

+             '</a>', output.data)
 

+ 

      @patch('pagure.lib.notify.send_email')
 

      def test_merge_request_pull_FF(self, send_email):
 

          """ Test the merge_request_pull endpoint with a FF PR. """
 
@@ -1422,6 +1429,12 @@ 
 

                  output.data)
 

              self.assertIn('<p>Test Initial Comment</p>', output.data)
 

  

+             # Test if the `open changed file icon` is displayed.
 

+             self.assertIn(
 

+                 'class="open_changed_file_icon_wrap"><span '
 

+                 'class="oi open_changed_file_icon" data-glyph="eye" '
 

+                 'alt="Open changed file" title="Open changed file"></span>'
 

+                 '</a>', output.data)
 

  

              # Case 2 - Add an empty initial comment
 

              data = {

When viewing a large diff in a PR, it can be helpful to see the
context of the changes. Display a small icon beside the diff's
changed lines, which when clicked will open the changed file (at
the relevant line number) in a new tab.

Implements ideas from https://pagure.io/pagure/issue/1753

What it looks like: https://paste.opensuse.org/view/simple/38571542

I found the eye icon most relevant, but it could be changed to something else if many of us feel differently. The icon is displayed only when hovering over the @@ -x,y +x,y @@ line in the diff.

Right now, this is implemented only for the PR page. Please point me to where else this needs to be added. We might also need to consider cases where the diff output is different. Since I'm fairly new, could someone please point out if there are any such cases?

Tests are still pending. I'll update them soon. Meanwhile, please suggest / correct :)

We already import flask above so there is no need for this one :)

You might want to use .partition() here to be safe (since you're not checking if this string is present in the line you're splitting)

I'm afraid including the color in there might make the split quite fragile :s

I agree, but I felt that was the most reliable way of making sure we weren't just confusing it for a line with the same text as the diff on it. What do you think we should have here?

Maybe just the last part? ; font-weight: bold"....

That ought to do it, I think, unless we make some changes in the html output. I'll change this.

2 new commits added

  • Add tests for 'open changed file' icon in PR diffs
  • Visual and logical improvements for 'jump to file from diff' feature
7 years ago

rebased
7 years ago

I applied the suggested changes, and made a couple of minor improvements. Can we have another round of review please?

rebased
7 years ago

Looks fine and works fine, let's rebase and merge :)

Since I have access to your fork, I did the rebase myself :)

rebased
7 years ago

Pull-Request has been merged by pingou
7 years ago
Changes Summary 4
+18 -0
file changed
pagure/static/pagure.css
+10 -0
file changed
pagure/templates/pull_request.html
+20 -0
file changed
pagure/ui/filters.py
+13 -0
file changed
tests/test_pagure_flask_ui_fork.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.