Add resource navigation buttons #293 #469

lf32 · 2022-07-22T06:57:12Z

Fixes #293

Fixes #293 Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

tdruez

See my comments.
Also, your JavaScript code style is inconsistent. Some lines ends with ; while some do not.
In doubt, try to follow the codebase conventions when contributing ;)

scanpipe/templates/scanpipe/resource_detail.html

tdruez · 2022-07-25T17:25:17Z

Also, the logic of the code is broken somehow.
If I use the buttons in a given section, "Licenses" for examples, everything works fine, but if you start going to other section, the prev/next is acting weird.

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

lf32 · 2022-07-28T15:04:52Z

Also, the logic of the code is broken somehow. If I use the buttons in a given section, "Licenses" for examples, everything works fine, but if you start going to other section, the prev/next is acting weird.

Yes @tdruez, got it fixed in the cd674fb commit

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

tdruez

The new buttons UI look good.

I think the "Next" button should be disabled if there's only 1 match.

tdruez · 2022-07-29T05:49:33Z

scanpipe/templates/scanpipe/resource_detail.html

        <p>
          {{ object.name }}
          | {{ object.size|filesizeformat }}
          | <a class="has-text-white" href="{{ object.get_raw_url }}?as_attachment=1"><i class="fas fa-download"></i></a>
          <br>
          <span style="font-size: 10px;">{{ object.path }}</span>
        </p>
+        <div>
+          <button class="button prev-btn is-dark" disabled onclick="previousValue();"><i class="fas fa-arrow-up"></i>&nbsp;Prev</button>


"Previous" is better than "Prev".

Sure @tdruez.

Fixed this.

tdruez · 2022-07-29T05:49:51Z

scanpipe/templates/scanpipe/resource_detail.html

        <p>
          {{ object.name }}
          | {{ object.size|filesizeformat }}
          | <a class="has-text-white" href="{{ object.get_raw_url }}?as_attachment=1"><i class="fas fa-download"></i></a>
          <br>
          <span style="font-size: 10px;">{{ object.path }}</span>
        </p>
+        <div>
+          <button class="button prev-btn is-dark" disabled onclick="previousValue();"><i class="fas fa-arrow-up"></i>&nbsp;Prev</button>


Inline JavaScript is not consistent with the rest of the codebase, keep the function but assign the click action using the addEventListener.

Added eventlisteners for buttons.

tdruez · 2022-07-29T05:50:04Z

scanpipe/templates/scanpipe/resource_detail.html

        <p>
          {{ object.name }}
          | {{ object.size|filesizeformat }}
          | <a class="has-text-white" href="{{ object.get_raw_url }}?as_attachment=1"><i class="fas fa-download"></i></a>
          <br>
          <span style="font-size: 10px;">{{ object.path }}</span>
        </p>
+        <div>
+          <button class="button prev-btn is-dark" disabled onclick="previousValue();"><i class="fas fa-arrow-up"></i>&nbsp;Prev</button>
+          <button class="button next-btn is-dark" disabled onclick="nextValue();"><i class="fas fa-arrow-down"></i>&nbsp;Next</button>


Do not use   for formatting/rendering. If you want some space between 2 elements, use the mr-* CSS classes.

Fixed this.

tdruez · 2022-07-29T05:51:01Z

scanpipe/templates/scanpipe/resource_detail.html

Please configure your editor to stop adding blank line for template files.

don't we need a blank line at the end of files?

Not for templates.

Ok then, I'll fix this.

Fixed this.

tdruez · 2022-07-29T06:01:14Z

scanpipe/templates/scanpipe/resource_detail.html

+        if (valueIndex >= detected_data.length - 1) return false;
+        valueIndex++;
+        editor.renderer.scrollToLine(detected_data[valueIndex].start_line - 1);
+        nextBtn.disabled = (valueIndex == detected_data.length - 1) ? true : false; 


Can be simplified to valueIndex == detected_data.length - 1.

Fixed this.

tdruez · 2022-07-29T06:01:26Z

scanpipe/templates/scanpipe/resource_detail.html

+        if (valueIndex <= 0) return false;
+        valueIndex--;
+        editor.renderer.scrollToLine(detected_data[valueIndex].start_line - 1);
+        prevBtn.disabled  = (valueIndex == 0) ? true : false;


Can be simplified to valueIndex == 0.

Fixed this.

tdruez · 2022-07-29T06:02:21Z

scanpipe/templates/scanpipe/resource_detail.html

@@ -102,28 +106,55 @@
      editor.getSession().setAnnotations(annotations);
    }

-    const $selectionButtons = getAll('.tabs a');
+    let valueIndex = 0;


scrollPositionIndex maybe?

Much better, fixed this.

tdruez · 2022-07-29T06:08:46Z

scanpipe/templates/scanpipe/resource_detail.html

+        mode: "ace/mode/text",
+        autoScrollEditorIntoView: true,
+        wrap: true,
+        readOnly: true,
+        showPrintMargin: false,
+        highlightActiveLine: false,
+        highlightGutterLine: false,
+        fontSize: 15,
+        foldStyle: "manual",
+        fontFamily: "SFMono-Regular,Consolas,Liberation Mono,Menlo,monospace",


Do not change existing style conventions but adapt to it.

Indentation using prettifier

Please revert and adapt to the existing style.
You're making the code review process more painful with those unwanted changes.

I have been using tab of 4 spaces, just switched to 2.

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

tdruez · 2022-07-29T08:41:31Z

Thanks for your contribution @lf32 !

lf32 · 2022-07-29T08:44:07Z

Thanks for the merge @tdruez!

Add resource navigation buttons #293

e56669f

Fixes #293 Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

lf32 mentioned this pull request Jul 22, 2022

An app to detect licenses from the provided input license text #450

Draft

tdruez requested changes Jul 25, 2022

View reviewed changes

scanpipe/templates/scanpipe/resource_detail.html Outdated Show resolved Hide resolved

scanpipe/templates/scanpipe/resource_detail.html Outdated Show resolved Hide resolved

scanpipe/templates/scanpipe/resource_detail.html Outdated Show resolved Hide resolved

Fix navigation buttons in resources #293

cd674fb

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

Add eventlisteners to buttons and fix js indentations #293

009af0c

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

tdruez requested changes Jul 29, 2022

View reviewed changes

tdruez reviewed Jul 29, 2022

View reviewed changes

lf32 added 2 commits July 29, 2022 11:59

Refine js variables #293

e6d56bf

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

Adapt js code indentation to match rest of the codebase #293

c0ae753

Signed-off-by: Akhil Raj <lf32.dev@gmail.com>

tdruez merged commit 7d084c7 into main Jul 29, 2022

tdruez deleted the resources-nav branch July 29, 2022 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource navigation buttons #293 #469

Add resource navigation buttons #293 #469

lf32 commented Jul 22, 2022

tdruez left a comment

tdruez commented Jul 25, 2022

lf32 commented Jul 28, 2022

tdruez left a comment •

edited

Loading

tdruez Jul 29, 2022

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading

tdruez Jul 29, 2022

lf32 Jul 29, 2022 •

edited

Loading

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez Jul 29, 2022

tdruez Jul 29, 2022

lf32 Jul 29, 2022 •

edited

Loading

tdruez Jul 29, 2022

lf32 Jul 29, 2022

tdruez commented Jul 29, 2022

lf32 commented Jul 29, 2022

Add resource navigation buttons #293 #469

Add resource navigation buttons #293 #469

Conversation

lf32 commented Jul 22, 2022

tdruez left a comment

Choose a reason for hiding this comment

tdruez commented Jul 25, 2022

lf32 commented Jul 28, 2022

tdruez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf32 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

lf32 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf32 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf32 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf32 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdruez commented Jul 29, 2022

lf32 commented Jul 29, 2022

tdruez left a comment •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading

lf32 Jul 29, 2022 •

edited

Loading