Feature/generalize tables

[zsloan]

This PR is intended to create a generic "create_table.js" that can be called for all pages that create DataTables. We've had many issues in the past with tables not sharing the same settings/appearances, so this should hopefully address that through having a shared set of default settings (that can be overwritten when necessary.

[BonfaceKilz]

@zsloan herein lies my review.

I'm Cc'ing different people, let's get into the habit of looking at - when a feature is complete - what different people are working. That's a quick way of (a) disseminating knowledge from the more experienced devs and (b) catching things that are off. You don't have to take much time on this - off hours shousd suffice to give a good-ish review. For GN2/3 work that should be merged - and one that's not a "hot" fix - let me/Zach have a quick skim before we merge. For other people, this PR can be found here:

<https://issues.genenetwork.org/issues/generalize-tables>

@robwwilliams scroll to the very end...

---

wqflask/wqflask/collect.py | 1 + 1 file changed, 1 insertion(+)

diff --git a/wqflask/wqflask/collect.py b/wqflask/wqflask/collect.py index 891da437f..b46e18592 100644 --- a/wqflask/wqflask/collect.py +++ b/wqflask/wqflask/collect.py @@ -285,6 +285,7 @@ def view_collection(): else: return render_template( "collections/view.html",

• traits_json=json_version, trait_info_str=trait_info_str, **collection_info)

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

03a92ba1b2ed0e79a74e5a791fdecf73304ee2e0 Mon Sep 17 00:00:00 2001

.../static/new/javascript/create_datatable.js | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 wqflask/wqflask/static/new/javascript/create_datatable.js

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js new file mode 100644 index 000000000..18a4f9d71 --- /dev/null +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -0,0 +1,92 @@ +create_table = function(tableId, tableData, columnDefs, customSettings) { +

• loadDataTable(tableId=tableId, tableFata=tableData, tableSettings=tableSettings, firstRun=true)

Nitpick: This has a side-effect - it doesn't return anything other than "do" something - within this closure. This smells like something to be re-worked in a separate PR.

• function loadDataTable(tableId, tableData, tableSettings, firstRun=false){
• if (!firstRun){
• setUserColumnsDefWidths(tableId);
• }

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

    tableSettings = {

• 'data': tableData,
• 'columns': columnDefs,
• "sDom": "iti",
• "destroy": true,
• "autoWidth": false,
• "bSortClasses": false,
• "scrollY": "100vh",
• "scrollCollapse": true,
• "scroller": true,
• "iDisplayLength": -1,
• "initComplete": function (settings) {
• // Add JQueryUI resizable functionality to each th in the ScrollHead table
• $('#' + table_id + '_wrapper .dataTables_scrollHead thead th').resizable({
• handles: "e",
• alsoResize: '#' + tableId + '_wrapper .dataTables_scrollHead table', //Not essential but makes the resizing smoother
• resize: function( event, ui ) {
• widthChange = ui.size.width - ui.originalSize.width;
• },
• stop: function () {
• saveColumnSettings(table_id, the_table);
• loadDataTable(first_run=false, table_id, table_data);

question: nitpick: This is an async recursive (?) call. Why?

• }
• });
• }
• }
• // Replace default settings with custom settings or add custom settings if not already set in default settings
• $.each(customSettings, function(key, value)) {
• tableSettings[key] = value
• }
• }

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

+ if (!firstRun){

• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); // Change the container width by the change in width of the adjusted column, so the overall table size adjusts properly

Hmmmm... In GN2 our CSS is a mess. And part of that is because we do alot of fiddling with javascript. You could do such calculations in CSS instead, and attach a class. Something like:

#tableID {
        width: calc(100% + 17px);
      }

See https://www.w3docs.com/snippets/css/how-to-calculate-the-width-of-an-element-with-the-css-calc-function.html for more.

Comment: Also, when writing comments, moreso for long lines - similar to Python - make them aesthetic. Keep in mind, some one - which may be a future version of yourself - may need/have to read this. Notice the difference:

// Change the container width by the change in width of the adjusted
// column, so the overall table size adjusts properly
$('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); 

• let checkedRows = getCheckedRows(tableId);
• theTable = $('#' + tableId).DataTable(tableSettings);
• if (checkedRows.length > 0){
• recheckRows(theTable, checkedRows);
• }

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

• } else {
• theTable = $('#' + tableId).DataTable(tableSettings);
• theTable.draw();
• }
• theTable.on( 'order.dt search.dt draw.dt', function () {
• theTable.column(1, {search:'applied', order:'applied'}).nodes().each( function (cell, i) {
• cell.innerHTML = i+1;
• } );
• } ).draw();

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

• if (firstRun){
• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + 17) + "px");
• }

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

[...]

From b1decd7288bb969d827ee04701df00e539935560 Mon Sep 17 00:00:00 2001 Subject: [PATCH 04/35] Change table_functions.js variables to camelCase

:+1: for this change.

From df02c35cc781c58e169c3574d78e00d1fea499f4 Mon Sep 17 00:00:00 2001 Subject: [PATCH 05/35] Pass table data to collections/view template and create_table.js

---

wqflask/wqflask/templates/collections/view.html | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 3ebee77c5..6d0e3edd4 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -188,8 +188,9 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

 <script type="text/javascript"
  src="/static/new/javascript/partial_correlations.js"></script>

-

comment: issue: You could define this...

 <script language="javascript" type="text/javascript">
     $(document).ready( function () {

... here. And use "let". "let" has it's advantanges, in particular limiting the scope of a variable to a block. Let's get into the habit of using - when feasible - let over var (OT: you should most def read the book: "Let Over Lambda")

@@ -217,7 +218,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record "order": [[1, "asc" ]] }

question: Does jinja support f-strings? If so, let's use those as a matter of preference.

• create_table(table_id, column_defs, table_settings)

• create_table(table_id, traits_json, column_defs, table_settings)

       submit_special = function(url) { 
          $("#collection_form").attr("action", url);

From a4836e8d5d79660c57099a23d306ff2b2f13be06 Mon Sep 17 00:00:00 2001 Subject: [PATCH 06/35] Change create_datatable.js from CRLF to LF + fix a couple syntax bugs

A more permanent solution for CRLF would be to have a .gitattributes file that enforces that for everybody*. @Zach could you look into that?

[*] https://www.aleksandrhovhannisyan.com/blog/crlf-vs-lf-normalizing-line-endings-in-git/

[...]

From 895ae3ae28703d939d218429d15768649a53f18a Mon Sep 17 00:00:00 2001 Subject: [PATCH 07/35] Convert trait.view to string for conversion to JSON

It's originally a boolean, which causes an error when passed to the JS code as JSON

wqflask/base/trait.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 11b28c5c5..0333e429d 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -302,7 +302,7 @@ def jsonable(trait, dataset=None):

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• view=trait.view,
• view=str(trait.view), symbol=trait.symbol, dataset=dataset.name, dataset_name=dataset.shortname, @@ -316,7 +316,7 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +332,7 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,14 +346,14 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, location=trait.location_repr ) elif dataset.name == "Temp": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset="Temp", dataset_name="Temp") else:

issue: important: Here, you convert a python boolean - True/False - to a string. The json spec uses true/false. Note the lowercase - a possible subtle case for a bug. Please change this, and make sure the corresponding js files work just fine. You could grep for the True/False in the js files to make your work easier.

From 65768f80a4d5ec0320ca62355e5dec8e112b85c8 Mon Sep 17 00:00:00 2001 Subject: [PATCH 08/35] Change jsonable in GeneralTrait so that it passes all necessary table information

---

wqflask/base/trait.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 0333e429d..b02c60336 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -300,3 +300,7 @@ def jsonable(trait, dataset=None): dataset_type=trait.dataset.type, group_name=trait.dataset.group.name)

• trait_symbol = "N/A"
• if trait.symbol:
• trait_symbol = trait.symbol

nitpick: not necessary but for one-liners like this, I find it easier to write something like:

     trait_symbol = trait.symbol if trait.symbol else "N/A"

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

nitpick: Please use f-strings here.

• symbol=trait.symbol,
• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -316,9 +322,12 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

See above.

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +341,10 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

Ditto.

                     view=str(trait.view),

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,6 +358,8 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, @@ -353,6 +367,8 @@ def jsonable(trait, dataset=None): ) elif dataset.name == "Temp": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset="Temp", dataset_name="Temp")

Ditto (wrt to f-strings).

Worth noting, something that Arun pointed out elsewhere, when we move to python > 3.10, we should prefer pattern-matching with "match" over "if ... else" statements. See https://peps.python.org/pep-0636/ for more info.

[...]

From d5c6bb7abfb3535c1fa5d0d8df95645a213ab030 Mon Sep 17 00:00:00 2001 Subject: [PATCH 10/35] Remove HTML table from collections/view.html and replace with list of column definitions passed to create_datatable.js

---

.../wqflask/templates/collections/view.html | 215 +++++++++++------- 1 file changed, 138 insertions(+), 77 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6d0e3edd4..6ea129f96 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html

[...]

• {% endfor %}
• 
  Loading...
  

nitpick: [you can ignore this] "
" is much better than "
". No good reason why...

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'description')){
• try {
• return decodeURIComponent(escape(data.description))
• } catch(err){
• return escape(data.description)
• }
• } else {
• return "N/A"
• }
• }

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'mean')){
• return data.mean.toFixed(3)
• } else {
• return "N/A"
• }
• }
• },

See above...

[...]

• 'title': "
  Peak
  LOD <a href=\"{{ url_for('glossary_blueprint.glossary') }}#LRS\" target=\"_blank\" style=\"color: white;\">^?
  ",

tip: nitpick: In cases like this, use single quotes to save yourself the chore of having to escape double-quotes.

• 'type': "natural-minus-na",
• 'data': null,
• 'width': "60px",
• 'targets': 8,
• 'orderSequence': [ "desc", "asc"],
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_score')){
• return (data.lrs_score / 4.61).toFixed(3)
• } else {
• return "N/A"
• }
• }

See above concern on extra args.

• {
• 'title': "
  Peak Location
  ",
• 'type': "natural-minus-na",
• 'data': null,
• 'width': "125px",
• 'targets': 9,
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_location')){
• return data.lrs_location
• } else {
• return "N/A"
• }
• }
• },

Ditto.

• {
• 'title': "
  Effect
  Size <a href=\"{{ url_for('glossary_blueprint.glossary') }}#A\" target=\"_blank\" style=\"color: white;\">^?
  ",

[...]

From 0ddd74a4d417389f7a35ac0971b0c17f5d79fb56 Mon Sep 17 00:00:00 2001 Subject: [PATCH 11/35] Change some variables in search_result_page.html to camelCase

+1 for this \m/\m/.

[...]

From ca368e32528073de15d925b4293b938f95f69dac Mon Sep 17 00:00:00 2001 Subject: [PATCH 12/35] Fix JS/CSS imports and include scroller setting in custom table settings

---

wqflask/wqflask/templates/collections/view.html | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6ea129f96..a12c13695 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -3,3 +3,4 @@ {% block css %}

 <link rel="stylesheet" type="text/css" href="{{ url_for('js', filename='DataTablesExtensions/buttonStyles/css/buttons.dataTables.min.css') }}">

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

[...]

From 2e0e3aaa2c78c355e83cfae21c9655554451200b Mon Sep 17 00:00:00 2001 Subject: [PATCH 14/35] Fix manual column width change feature to work in create_datatable.js

---

.../static/new/javascript/create_datatable.js | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 7b495a8b7..217e99be6 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -2,9 +2,11 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

 loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

nitpick: prefer var over let. And this looks much nicer.

• // Store the change in width so overall table width can be adjusted by
• // that amount
• let widthChange = 0;

It's - sometimes - nice to have [literally] pretty code. It: (a) encourages contribs (b) Makes life easy when reviewing (c) Makes it easy when troubleshooting/debugging

nitpick: Of note, for comments, similar to git commits, using the imperative mood - sometimes - reads better. Also, try not to use append comments with your name. This usually tends to have a nasty effect on a person who would want to change something you wrote; something along the lines of 'I shouldn't touch this!'. Imagine if saw something like this sometime in the future:

// Munyoki: some comments follow...
let someConf = 2

What would you think if you were supposed to fiddle with that? At best, git blame allows you to find out who did what where.

[...]

From f22983a304d0c621ef3dab9aac8e824911e66485 Mon Sep 17 00:00:00 2001 Subject: [PATCH 15/35] Change JS variables to camelCase and assign ID to table container div

---

wqflask/wqflask/templates/collections/view.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index a12c13695..2349e16da 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

[...]

• create_table(table_id, traits_json, column_defs, table_settings)
• create_table(tableId, traitsJson, columnDefs, tableSettings)

+1 for using camelCase all over. Perhaps in this case, you do have a createTable too.

[...]

From 0ca935b8abda975334b89673ccef4bcd55837773 Mon Sep 17 00:00:00 2001 Subject: [PATCH 16/35] Added createdRow callback function to custom table settings for View Collection page

Also set a default table width of 2000px

.../wqflask/templates/collections/view.html | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 2349e16da..8dee52444 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

See above comment on putting things in CSS...

[...]

From 8a646b232b144323989f5cb701d18de4745920ba Mon Sep 17 00:00:00 2001 Subject: [PATCH 17/35] Fix issue where customSettings weren't being passed when manually adjusting column widths

---

wqflask/wqflask/static/new/javascript/create_datatable.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 217e99be6..2a895e185 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -1,10 +1,9 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

• loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• loadDataTable(tableId=tableId, tableData=tableData, customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

• var widthChange = 0; // For storing the change in width so overall table width can be adjusted by that amount function loadDataTable(tableId, tableData, customSettings, firstRun=false){

See above comment no var/let/comments.

[...]

From 714b64d8830cd5d4ca06cb07c162038155e56675 Mon Sep 17 00:00:00 2001 Subject: [PATCH 18/35] Change search_result_page.html to generate its table with create_datatable.js

---

.../wqflask/templates/search_result_page.html | 583 ++++++++---------- 1 file changed, 245 insertions(+), 338 deletions(-)

diff --git a/wqflask/wqflask/templates/search_result_page.html b/wqflask/wqflask/templates/search_result_page.html index fb7f404bf..0bce6793d 100644 --- a/wqflask/wqflask/templates/search_result_page.html +++ b/wqflask/wqflask/templates/search_result_page.html @@ -131,7 +131,7 @@ {% endif %}

[BonfaceKilz]

Hi Zach,

I hope you don't mind the review. We have been doing this internally, so it is not personal. I think it is great Bonface is very critical.

Just take what you think makes sense :)

Pj.

On Sat, Jul 30, 2022 at 08:01:56PM +0300, Munyoki Kilyungi wrote:

@Zach herein lies my review.

I'm Cc'ing different people, let's get into the habit of looking at - when a feature is complete - what different people are working. That's a quick way of (a) disseminating knowledge from the more experienced devs and (b) catching things that are off. You don't have to take much time on this - off hours shousd suffice to give a good-ish review. For GN2/3 work that should be merged - and one that's not a "hot" fix - let me/Zach have a quick skim before we merge. For other people, this PR can be found here:

<https://issues.genenetwork.org/issues/generalize-tables>

@Rob scroll to the very end...

---

wqflask/wqflask/collect.py | 1 + 1 file changed, 1 insertion(+)

diff --git a/wqflask/wqflask/collect.py b/wqflask/wqflask/collect.py index 891da437f..b46e18592 100644 --- a/wqflask/wqflask/collect.py +++ b/wqflask/wqflask/collect.py @@ -285,6 +285,7 @@ def view_collection(): else: return render_template( "collections/view.html",

• traits_json=json_version, trait_info_str=trait_info_str, **collection_info)

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

03a92ba1b2ed0e79a74e5a791fdecf73304ee2e0 Mon Sep 17 00:00:00 2001

.../static/new/javascript/create_datatable.js | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 wqflask/wqflask/static/new/javascript/create_datatable.js

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js new file mode 100644 index 000000000..18a4f9d71 --- /dev/null +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -0,0 +1,92 @@ +create_table = function(tableId, tableData, columnDefs, customSettings) { +

• loadDataTable(tableId=tableId, tableFata=tableData, tableSettings=tableSettings, firstRun=true)

Nitpick: This has a side-effect - it doesn't return anything other than "do" something - within this closure. This smells like something to be re-worked in a separate PR.

• function loadDataTable(tableId, tableData, tableSettings, firstRun=false){
• if (!firstRun){
• setUserColumnsDefWidths(tableId);
• }

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

    tableSettings = {

• 'data': tableData,
• 'columns': columnDefs,
• "sDom": "iti",
• "destroy": true,
• "autoWidth": false,
• "bSortClasses": false,
• "scrollY": "100vh",
• "scrollCollapse": true,
• "scroller": true,
• "iDisplayLength": -1,
• "initComplete": function (settings) {
• // Add JQueryUI resizable functionality to each th in the ScrollHead table
• $('#' + table_id + '_wrapper .dataTables_scrollHead thead th').resizable({
• handles: "e",
• alsoResize: '#' + tableId + '_wrapper .dataTables_scrollHead table', //Not essential but makes the resizing smoother
• resize: function( event, ui ) {
• widthChange = ui.size.width - ui.originalSize.width;
• },
• stop: function () {
• saveColumnSettings(table_id, the_table);
• loadDataTable(first_run=false, table_id, table_data);

question: nitpick: This is an async recursive (?) call. Why?

• }
• });
• }
• }
• // Replace default settings with custom settings or add custom settings if not already set in default settings
• $.each(customSettings, function(key, value)) {
• tableSettings[key] = value
• }
• }

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

+ if (!firstRun){

• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); // Change the container width by the change in width of the adjusted column, so the overall table size adjusts properly

Hmmmm... In GN2 our CSS is a mess. And part of that is because we do alot of fiddling with javascript. You could do such calculations in CSS instead, and attach a class. Something like:

#tableID {
        width: calc(100% + 17px);
      }

See https://www.w3docs.com/snippets/css/how-to-calculate-the-width-of-an-element-with-the-css-calc-function.html for more.

Comment: Also, when writing comments, moreso for long lines - similar to Python - make them aesthetic. Keep in mind, some one - which may be a future version of yourself - may need/have to read this. Notice the difference:

// Change the container width by the change in width of the adjusted
// column, so the overall table size adjusts properly
$('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); 

• let checkedRows = getCheckedRows(tableId);
• theTable = $('#' + tableId).DataTable(tableSettings);
• if (checkedRows.length > 0){
• recheckRows(theTable, checkedRows);
• }

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

• } else {
• theTable = $('#' + tableId).DataTable(tableSettings);
• theTable.draw();
• }
• theTable.on( 'order.dt search.dt draw.dt', function () {
• theTable.column(1, {search:'applied', order:'applied'}).nodes().each( function (cell, i) {
• cell.innerHTML = i+1;
• } );
• } ).draw();

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

• if (firstRun){
• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + 17) + "px");
• }

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

[...]

From b1decd7288bb969d827ee04701df00e539935560 Mon Sep 17 00:00:00 2001 Subject: [PATCH 04/35] Change table_functions.js variables to camelCase

:+1: for this change.

From df02c35cc781c58e169c3574d78e00d1fea499f4 Mon Sep 17 00:00:00 2001 Subject: [PATCH 05/35] Pass table data to collections/view template and create_table.js

---

wqflask/wqflask/templates/collections/view.html | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 3ebee77c5..6d0e3edd4 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -188,8 +188,9 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

 <script type="text/javascript"
    src="/static/new/javascript/partial_correlations.js"></script>

-

comment: issue: You could define this...

 <script language="javascript" type="text/javascript">
     $(document).ready( function () {

... here. And use "let". "let" has it's advantanges, in particular limiting the scope of a variable to a block. Let's get into the habit of using - when feasible - let over var (OT: you should most def read the book: "Let Over Lambda")

@@ -217,7 +218,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record "order": [[1, "asc" ]] }

question: Does jinja support f-strings? If so, let's use those as a matter of preference.

• create_table(table_id, column_defs, table_settings)

• create_table(table_id, traits_json, column_defs, table_settings)

       submit_special = function(url) { 
          $("#collection_form").attr("action", url);

From a4836e8d5d79660c57099a23d306ff2b2f13be06 Mon Sep 17 00:00:00 2001 Subject: [PATCH 06/35] Change create_datatable.js from CRLF to LF + fix a couple syntax bugs

A more permanent solution for CRLF would be to have a .gitattributes file that enforces that for everybody*. @Zach could you look into that?

[*] https://www.aleksandrhovhannisyan.com/blog/crlf-vs-lf-normalizing-line-endings-in-git/

[...]

From 895ae3ae28703d939d218429d15768649a53f18a Mon Sep 17 00:00:00 2001 Subject: [PATCH 07/35] Convert trait.view to string for conversion to JSON

It's originally a boolean, which causes an error when passed to the JS code as JSON

wqflask/base/trait.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 11b28c5c5..0333e429d 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -302,7 +302,7 @@ def jsonable(trait, dataset=None):

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• view=trait.view,
• view=str(trait.view), symbol=trait.symbol, dataset=dataset.name, dataset_name=dataset.shortname, @@ -316,7 +316,7 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +332,7 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,14 +346,14 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, location=trait.location_repr ) elif dataset.name == "Temp": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset="Temp", dataset_name="Temp") else:

issue: important: Here, you convert a python boolean - True/False - to a string. The json spec uses true/false. Note the lowercase - a possible subtle case for a bug. Please change this, and make sure the corresponding js files work just fine. You could grep for the True/False in the js files to make your work easier.

From 65768f80a4d5ec0320ca62355e5dec8e112b85c8 Mon Sep 17 00:00:00 2001 Subject: [PATCH 08/35] Change jsonable in GeneralTrait so that it passes all necessary table information

---

wqflask/base/trait.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 0333e429d..b02c60336 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -300,3 +300,7 @@ def jsonable(trait, dataset=None): dataset_type=trait.dataset.type, group_name=trait.dataset.group.name)

• trait_symbol = "N/A"
• if trait.symbol:
• trait_symbol = trait.symbol

nitpick: not necessary but for one-liners like this, I find it easier to write something like:

     trait_symbol = trait.symbol if trait.symbol else "N/A"

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

nitpick: Please use f-strings here.

• symbol=trait.symbol,
• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -316,9 +322,12 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

See above.

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +341,10 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

Ditto.

                     view=str(trait.view),

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,6 +358,8 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, @@ -353,6 +367,8 @@ def jsonable(trait, dataset=None): ) elif dataset.name == "Temp": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset="Temp", dataset_name="Temp")

Ditto (wrt to f-strings).

Worth noting, something that Arun pointed out elsewhere, when we move to python > 3.10, we should prefer pattern-matching with "match" over "if ... else" statements. See https://peps.python.org/pep-0636/ for more info.

[...]

From d5c6bb7abfb3535c1fa5d0d8df95645a213ab030 Mon Sep 17 00:00:00 2001 Subject: [PATCH 10/35] Remove HTML table from collections/view.html and replace with list of column definitions passed to create_datatable.js

---

.../wqflask/templates/collections/view.html | 215 +++++++++++------- 1 file changed, 138 insertions(+), 77 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6d0e3edd4..6ea129f96 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html

[...]

• {% endfor %}
• 
  Loading...
  

nitpick: [you can ignore this] "
" is much better than "
". No good reason why...

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'description')){
• try {
• return decodeURIComponent(escape(data.description))
• } catch(err){
• return escape(data.description)
• }
• } else {
• return "N/A"
• }
• }

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'mean')){
• return data.mean.toFixed(3)
• } else {
• return "N/A"
• }
• }
• },

See above...

[...]

• 'title': "
  Peak
  LOD <a href=\"{{ url_for('glossary_blueprint.glossary') }}#LRS\" target=\"_blank\" style=\"color: white;\">^?
  ",

tip: nitpick: In cases like this, use single quotes to save yourself the chore of having to escape double-quotes.

• 'type': "natural-minus-na",
• 'data': null,
• 'width': "60px",
• 'targets': 8,
• 'orderSequence': [ "desc", "asc"],
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_score')){
• return (data.lrs_score / 4.61).toFixed(3)
• } else {
• return "N/A"
• }
• }

See above concern on extra args.

• {
• 'title': "
  Peak Location
  ",
• 'type': "natural-minus-na",
• 'data': null,
• 'width': "125px",
• 'targets': 9,
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_location')){
• return data.lrs_location
• } else {
• return "N/A"
• }
• }
• },

Ditto.

• {
• 'title': "
  Effect
  Size <a href=\"{{ url_for('glossary_blueprint.glossary') }}#A\" target=\"_blank\" style=\"color: white;\">^?
  ",

[...]

From 0ddd74a4d417389f7a35ac0971b0c17f5d79fb56 Mon Sep 17 00:00:00 2001 Subject: [PATCH 11/35] Change some variables in search_result_page.html to camelCase

+1 for this \m/\m/.

[...]

From ca368e32528073de15d925b4293b938f95f69dac Mon Sep 17 00:00:00 2001 Subject: [PATCH 12/35] Fix JS/CSS imports and include scroller setting in custom table settings

---

wqflask/wqflask/templates/collections/view.html | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6ea129f96..a12c13695 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -3,3 +3,4 @@ {% block css %}

 <link rel="stylesheet" type="text/css" href="{{ url_for('js', filename='DataTablesExtensions/buttonStyles/css/buttons.dataTables.min.css') }}">

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

[...]

From 2e0e3aaa2c78c355e83cfae21c9655554451200b Mon Sep 17 00:00:00 2001 Subject: [PATCH 14/35] Fix manual column width change feature to work in create_datatable.js

---

.../static/new/javascript/create_datatable.js | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 7b495a8b7..217e99be6 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -2,9 +2,11 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

 loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

nitpick: prefer var over let. And this looks much nicer.

• // Store the change in width so overall table width can be adjusted by
• // that amount
• let widthChange = 0;

It's - sometimes - nice to have [literally] pretty code. It: (a) encourages contribs (b) Makes life easy when reviewing (c) Makes it easy when troubleshooting/debugging

nitpick: Of note, for comments, similar to git commits, using the imperative mood - sometimes - reads better. Also, try not to use append comments with your name. This usually tends to have a nasty effect on a person who would want to change something you wrote; something along the lines of 'I shouldn't touch this!'. Imagine if saw something like this sometime in the future:

// Munyoki: some comments follow...
let someConf = 2

What would you think if you were supposed to fiddle with that? At best, git blame allows you to find out who did what where.

[...]

From f22983a304d0c621ef3dab9aac8e824911e66485 Mon Sep 17 00:00:00 2001 Subject: [PATCH 15/35] Change JS variables to camelCase and assign ID to table container div

---

wqflask/wqflask/templates/collections/view.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index a12c13695..2349e16da 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

[...]

• create_table(table_id, traits_json, column_defs, table_settings)
• create_table(tableId, traitsJson, columnDefs, tableSettings)

+1 for using camelCase all over. Perhaps in this case, you do have a createTable too.

[...]

From 0ca935b8abda975334b89673ccef4bcd55837773 Mon Sep 17 00:00:00 2001 Subject: [PATCH 16/35] Added createdRow callback function to custom table settings for View Collection page

Also set a default table width of 2000px

.../wqflask/templates/collections/view.html | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 2349e16da..8dee52444 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

See above comment on putting things in CSS...

[...]

From 8a646b232b144323989f5cb701d18de4745920ba Mon Sep 17 00:00:00 2001 Subject: [PATCH 17/35] Fix issue where customSettings weren't being passed when manually adjusting column widths

---

wqflask/wqflask/static/new/javascript/create_datatable.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 217e99be6..2a895e185 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -1,10 +1,9 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

• loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• loadDataTable(tableId=tableId, tableData=tableData, customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

• var widthChange = 0; // For storing the change in width so overall table width can be adjusted by that amount function loadDataTable(tableId, tableData, customSettings, firstRun=false){

See above comment no var/let/comments.

[...]

From 714b64d8830cd5d4ca06cb07c162038155e56675 Mon Sep 17 00:00:00 2001 Subject: [PATCH 18/35] Change search_result_page.html to generate its table with create_datatable.js

---

.../wqflask/templates/search_result_page.html | 583 ++++++++---------- 1 file changed, 245 insertions(+), 338 deletions(-)

diff --git a/wqflask/wqflask/templates/search_result_page.html b/wqflask/wqflask/templates/search_result_page.html index fb7f404bf..0bce6793d 100644 --- a/wqflask/wqflask/templates/search_result_page.html +++ b/wqflask/wqflask/templates/search_result_page.html @@ -131,7 +131,7 @@ {% endif %}

[zsloan]

Thanks for the review! I appreciate you taking the time to look through all of this.

I have no idea why the commented out code was still in initialize_show_trait_tables.js; must have just forgotten to delete that (for each file I would initially comment out the code I was trying to replace, and I must have just forgotten to remove it before committing with that file). Also, the comment with the "ZS" is from some older code copied over, so I'll just remove that; I don't do that anymore with new comments. I'm not sure how to make a PR like this much smaller unless I just did a separate PR for each table, though; separating out stuff like converting variables to camelCase is a good idea (as you say) but most of these commits are still related to the actual table changes (and I frequently encountered issue that required me to go back and further edit create_datatable.js).

I already spoke with Rob about what to look for in testing (as you say, it's mostly just that functionality is the same).

I'll fix most of these things Monday, but a some answers to the questions (plus things I have questions about):

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

Currently this is because the table loading looks bad if there are a very large number of traits in the collection and it has to load the whole table into the DOM. If passed as JSON, it only ever has to render a limited number of rows using Scroller, but if passed as HTML it has to render all of the rows before DataTables "converts" the table into one with its functionality (which can look weird with big tables, since it can take a second or two in those cases). There might be an alternate way to do this, but this is why it currently does it this way instead of just using a for-loop in the template to draw rows.

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

question: nitpick: This is an async recursive (?) call. Why?

This is because it runs any time columns manually have their width adjusted through clicking/dragging (since it needs to redraw the table). You can see where it's called in the "initComplete" table setting.

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

Could you explain what you mean here?

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

I don't think there's another function called checkRows. What are you referring to?

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

I agree, but I'm not sure what to call it. Just "table" feels too generic and not obviously a variable name at a glance. "thisTable" maybe, though it's kind of similar to "theTable"

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

Yeah, no idea why I didn't just put that inside the Else of the If/Else directly before it (assuming that's what you're referring to).

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

There probably isn't one; I was just matching the syntax in the DataTables documentation, but I'll see if it works without all of those parameters passed (I'm guessing it should as long as I specify which one I'm passing).

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

You're correct; I added it like this for testing and just forgot to use the one in Guix (I'm pretty sure it actually is packaged, but if it isn't I'll let one of you know).

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

In a situation where a styling is only applied to a single element, should I just add some extra class (like "search-table" or "correlation-table" or whatever - the problem is that the widths vary depending on the table, so there'd need to be a separate one for each type)?

Widths are also often conditional on something - should there just be separate class names for each condition in the CSS? (this also comes up in another comment below)

This: "{% if corr_method == 'pearson' %}r{% else %}rho{% endif %}" is repetitive. How about setting it once somewhere and using that value in the above snip.

question: Shouldn't we do this in Python?

These two are kind of pertaining to a similar thing. Regarding the first, I agree that it's probably best to just define "rOrRho" (or whatever) somewhere earlier and then use that variable instead of the jinja2 conditional, so I'll make that change.

Regarding the latter (where you're commenting on the author_string being truncated with "et. al."), the reasoning I was using (which might be wrong) is to put things pertaining solely to the display in the template/JS. The alternative here would be to store a separate "authors_display" (or something) in the JSON (since I still want to pass the full authors list so it can be displayed when the user hovers their cursor over the cell). I think I'll go ahead and make this change (storing "authors_display" in the JSON row data), since if nothing else it's probably faster than doing it in the JS for every cell in the column.

 +                'title': "Description",
 +                'type': "natural",
 +                {% if (max_widths.description * 7) < 500 %}
 +                'width': "{{ max_widths.description * 7 }}px",
 +                {% else %}
 +                'width': "500px",
 +                {% endif %}
 +                'data': null,
 +                'targets': 3,
 +                'render': function(data, type, row, meta) {
 +                  try {
 +                      return decodeURIComponent(escape(data.description))
 +                  } catch(err){
 +                      return data.description
 +                  }
                  }

See above comments on CSS + extra unused args.

I'm not sure how to deal with conditionals like this in the CSS, other than just having separate very specific class names (like "wide-description-column" and "short-description-column"; also, the column widths can vary on the specific table, so there'd either need to be separate CSS files for each of these tables or more variables variable names). Is this what you have in mind?

[robwwilliams]

Great discussion!

On Sat, Jul 30, 2022 at 14:55 zsloan @.***> wrote:

Thanks for the review! I appreciate you taking the time to look through all of this.

I have no idea why the commented out code was still in initialize_show_trait_tables.js; must have just forgotten to delete that (for each file I would initially comment out the code I was trying to replace, and I must have just forgotten to remove it before committing with that file). Also, the comment with the "ZS" is from some older code copied over, so I'll just remove that; I don't do that anymore with new comments. I'm not sure how to make a PR like this much smaller unless I just did a separate PR for each table, though; separating out stuff like converting variables to camelCase is a good idea (as you say) but most of these commits are still related to the actual table changes (and I frequently encountered issue that required me to go back and further edit create_datatable.js).

I already spoke with Rob about what to look for in testing (as you say, it's mostly just that functionality is the same).

I'll fix most of these things Monday, but a some answers to the questions (plus things I have questions about):

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

Currently this is because the table loading looks bad if there are a very large number of traits in the collection and it has to load the whole table into the DOM. If passed as JSON, it only ever has to render a limited number of rows using Scroller, but if passed as HTML it has to render all of the rows before DataTables "converts" the table into one with its functionality (which can look weird with big tables, since it can take a second or two in those cases). There might be an alternate way to do this, but this is why it currently does it this way instead of just using a for-loop in the template to draw rows.

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

question: nitpick: This is an async recursive (?) call. Why?

This is because it runs any time columns manually have their width adjusted through clicking/dragging (since it needs to redraw the table). You can see where it's called in the "initComplete" table setting.

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

Could you explain what you mean here?

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

I don't think there's another function called checkRows. What are you referring to?

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

I agree, but I'm not sure what to call it. Just "table" feels too generic and not obviously a variable name at a glance. "thisTable" maybe, though it's kind of similar to "theTable"

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

Yeah, no idea why I didn't just put that inside the Else of the If/Else directly before it (assuming that's what you're referring to).

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

There probably isn't one; I was just matching the syntax in the DataTables documentation, but I'll see if it works without all of those parameters passed (I'm guessing it should as long as I specify which one I'm passing).

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

You're correct; I added it like this for testing and just forgot to use the one in Guix (I'm pretty sure it actually is packaged, but if it isn't I'll let one of you know).

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

In a situation where a styling is only applied to a single element, should I just add some extra class (like "search-table" or "correlation-table" or whatever - the problem is that the widths vary depending on the table, so there'd need to be a separate one for each type)?

Widths are also often conditional on something - should there just be separate class names for each condition in the CSS? (this also comes up in another comment below)

This: "{% if corr_method == 'pearson' %}r{% else %}rho{% endif %}" is repetitive. How about setting it once somewhere and using that value in the above snip.

question: Shouldn't we do this in Python?

These two are kind of pertaining to a similar thing. Regarding the first, I agree that it's probably best to just define "rOrRho" (or whatever) somewhere earlier and then use that variable instead of the jinja2 conditional, so I'll make that change.

Regarding the latter (where you're commenting on the author_string being truncated with "et. al."), the reasoning I was using (which might be wrong) is to put things pertaining solely to the display in the template/JS. The alternative here would be to store a separate "authors_display" (or something) in the JSON (since I still want to pass the full authors list so it can be displayed when the user hovers their cursor over the cell). I think I'll go ahead and make this change (storing "authors_display" in the JSON row data), since if nothing else it's probably faster than doing it in the JS for every cell in the column.

-

          'title': "Description",

-

          'type': "natural",

-

          {% if (max_widths.description * 7) < 500 %}

-

          'width': "{{ max_widths.description * 7 }}px",

-

          {% else %}

-

          'width': "500px",

-

          {% endif %}

-

          'data': null,

-

          'targets': 3,

-

          'render': function(data, type, row, meta) {

-

            try {

-

                return decodeURIComponent(escape(data.description))

-

            } catch(err){

-

                return data.description

-

            }
          }

See above comments on CSS + extra unused args.

I'm not sure how to deal with conditionals like this in the CSS, other than just having separate very specific class names (like "wide-description-column" and "short-description-column"; also, the column widths can vary on the specific table, so there'd either need to be separate CSS files for each of these tables or more variables variable names). Is this what you have in mind?

— Reply to this email directly, view it on GitHub https://github.com/genenetwork/genenetwork2/pull/713#issuecomment-1200284823, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC55V7AYSTWT2ISLFA2XNULVWWCCXANCNFSM5ZAAZEGQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Rob

Robert W Williams PhD Department of Genetics, Genomics and Informatics 71 S Manassas St, Memphis TN 38163 University of Tennessee Health Science Center, TSRB 405 Cell: 901 604 4752 EMAIL: @.*** Zoom: https://tennesseehipaa.zoom.us/j/9492269658

[BonfaceKilz]

Pjotr Prins @.***> aliandika:

Hi Zach,

I hope you don't mind the review. We have been doing this internally, so it is not personal. I think it is great Bonface is very critical.

I agree. We should create that culture here - that way GN2/3 becomes what we envision it to be.

Just take what you think makes sense :)

Pj.

On Sat, Jul 30, 2022 at 08:01:56PM +0300, Munyoki Kilyungi wrote:

@Zach herein lies my review.

I'm Cc'ing different people, let's get into the habit of looking at - when a feature is complete - what different people are working. That's a quick way of (a) disseminating knowledge from the more experienced devs and (b) catching things that are off. You don't have to take much time on this - off hours shousd suffice to give a good-ish review. For GN2/3 work that should be merged - and one that's not a "hot" fix - let me/Zach have a quick skim before we merge. For other people, this PR can be found here:

<https://issues.genenetwork.org/issues/generalize-tables>

@Rob scroll to the very end...

---

wqflask/wqflask/collect.py | 1 + 1 file changed, 1 insertion(+)

diff --git a/wqflask/wqflask/collect.py b/wqflask/wqflask/collect.py index 891da437f..b46e18592 100644 --- a/wqflask/wqflask/collect.py +++ b/wqflask/wqflask/collect.py @@ -285,6 +285,7 @@ def view_collection(): else: return render_template( "collections/view.html",

• traits_json=json_version, trait_info_str=trait_info_str, **collection_info)

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

03a92ba1b2ed0e79a74e5a791fdecf73304ee2e0 Mon Sep 17 00:00:00 2001

.../static/new/javascript/create_datatable.js | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 wqflask/wqflask/static/new/javascript/create_datatable.js

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js new file mode 100644 index 000000000..18a4f9d71 --- /dev/null +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -0,0 +1,92 @@ +create_table = function(tableId, tableData, columnDefs, customSettings) { +

• loadDataTable(tableId=tableId, tableFata=tableData, tableSettings=tableSettings, firstRun=true)

Nitpick: This has a side-effect - it doesn't return anything other than "do" something - within this closure. This smells like something to be re-worked in a separate PR.

• function loadDataTable(tableId, tableData, tableSettings, firstRun=false){
• if (!firstRun){
• setUserColumnsDefWidths(tableId);
• }

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

    tableSettings = {

• 'data': tableData,
• 'columns': columnDefs,
• "sDom": "iti",
• "destroy": true,
• "autoWidth": false,
• "bSortClasses": false,
• "scrollY": "100vh",
• "scrollCollapse": true,
• "scroller": true,
• "iDisplayLength": -1,
• "initComplete": function (settings) {
• // Add JQueryUI resizable functionality to each th in the ScrollHead table
• $('#' + table_id + '_wrapper .dataTables_scrollHead thead th').resizable({
• handles: "e",
• alsoResize: '#' + tableId + '_wrapper .dataTables_scrollHead table', //Not essential but makes the resizing smoother
• resize: function( event, ui ) {
• widthChange = ui.size.width - ui.originalSize.width;
• },
• stop: function () {
• saveColumnSettings(table_id, the_table);
• loadDataTable(first_run=false, table_id, table_data);

question: nitpick: This is an async recursive (?) call. Why?

• }
• });
• }
• }
• // Replace default settings with custom settings or add custom settings if not already set in default settings
• $.each(customSettings, function(key, value)) {
• tableSettings[key] = value
• }
• }

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

+ if (!firstRun){

• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); // Change the container width by the change in width of the adjusted column, so the overall table size adjusts properly

Hmmmm... In GN2 our CSS is a mess. And part of that is because we do alot of fiddling with javascript. You could do such calculations in CSS instead, and attach a class. Something like:

#tableID {
        width: calc(100% + 17px);
      }

See https://www.w3docs.com/snippets/css/how-to-calculate-the-width-of-an-element-with-the-css-calc-function.html for more.

Comment: Also, when writing comments, moreso for long lines - similar to Python - make them aesthetic. Keep in mind, some one - which may be a future version of yourself - may need/have to read this. Notice the difference:

// Change the container width by the change in width of the adjusted
// column, so the overall table size adjusts properly
$('#' + tableId + '_container').css("width", String($('#' + tableId).width() + widthChange + 17) + "px"); 

• let checkedRows = getCheckedRows(tableId);
• theTable = $('#' + tableId).DataTable(tableSettings);
• if (checkedRows.length > 0){
• recheckRows(theTable, checkedRows);
• }

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

• } else {
• theTable = $('#' + tableId).DataTable(tableSettings);
• theTable.draw();
• }
• theTable.on( 'order.dt search.dt draw.dt', function () {
• theTable.column(1, {search:'applied', order:'applied'}).nodes().each( function (cell, i) {
• cell.innerHTML = i+1;
• } );
• } ).draw();

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

• if (firstRun){
• $('#' + tableId + '_container').css("width", String($('#' + tableId).width() + 17) + "px");
• }

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

[...]

From b1decd7288bb969d827ee04701df00e539935560 Mon Sep 17 00:00:00 2001 Subject: [PATCH 04/35] Change table_functions.js variables to camelCase

:+1: for this change.

From df02c35cc781c58e169c3574d78e00d1fea499f4 Mon Sep 17 00:00:00 2001 Subject: [PATCH 05/35] Pass table data to collections/view template and create_table.js

---

wqflask/wqflask/templates/collections/view.html | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 3ebee77c5..6d0e3edd4 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -188,8 +188,9 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

 <script type="text/javascript"
   src="/static/new/javascript/partial_correlations.js"></script>

-

comment: issue: You could define this...

 <script language="javascript" type="text/javascript">
     $(document).ready( function () {

... here. And use "let". "let" has it's advantanges, in particular limiting the scope of a variable to a block. Let's get into the habit of using - when feasible - let over var (OT: you should most def read the book: "Let Over Lambda")

@@ -217,7 +218,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record "order": [[1, "asc" ]] }

question: Does jinja support f-strings? If so, let's use those as a matter of preference.

• create_table(table_id, column_defs, table_settings)

• create_table(table_id, traits_json, column_defs, table_settings)

       submit_special = function(url) { 
          $("#collection_form").attr("action", url);

From a4836e8d5d79660c57099a23d306ff2b2f13be06 Mon Sep 17 00:00:00 2001 Subject: [PATCH 06/35] Change create_datatable.js from CRLF to LF + fix a couple syntax bugs

A more permanent solution for CRLF would be to have a .gitattributes file that enforces that for everybody*. @Zach could you look into that?

[*] https://www.aleksandrhovhannisyan.com/blog/crlf-vs-lf-normalizing-line-endings-in-git/

[...]

From 895ae3ae28703d939d218429d15768649a53f18a Mon Sep 17 00:00:00 2001 Subject: [PATCH 07/35] Convert trait.view to string for conversion to JSON

It's originally a boolean, which causes an error when passed to the JS code as JSON

wqflask/base/trait.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 11b28c5c5..0333e429d 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -302,7 +302,7 @@ def jsonable(trait, dataset=None):

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• view=trait.view,
• view=str(trait.view), symbol=trait.symbol, dataset=dataset.name, dataset_name=dataset.shortname, @@ -316,7 +316,7 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +332,7 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,14 +346,14 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, location=trait.location_repr ) elif dataset.name == "Temp": return dict(name=trait.name,
• view=trait.view,
• view=str(trait.view), dataset="Temp", dataset_name="Temp") else:

issue: important: Here, you convert a python boolean - True/False - to a string. The json spec uses true/false. Note the lowercase - a possible subtle case for a bug. Please change this, and make sure the corresponding js files work just fine. You could grep for the True/False in the js files to make your work easier.

From 65768f80a4d5ec0320ca62355e5dec8e112b85c8 Mon Sep 17 00:00:00 2001 Subject: [PATCH 08/35] Change jsonable in GeneralTrait so that it passes all necessary table information

---

wqflask/base/trait.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/wqflask/base/trait.py b/wqflask/base/trait.py index 0333e429d..b02c60336 100644 --- a/wqflask/base/trait.py +++ b/wqflask/base/trait.py @@ -300,3 +300,7 @@ def jsonable(trait, dataset=None): dataset_type=trait.dataset.type, group_name=trait.dataset.group.name)

• trait_symbol = "N/A"
• if trait.symbol:
• trait_symbol = trait.symbol

nitpick: not necessary but for one-liners like this, I find it easier to write something like:

     trait_symbol = trait.symbol if trait.symbol else "N/A"

 if dataset.type == "ProbeSet":
     return dict(name=trait.name,

• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

nitpick: Please use f-strings here.

• symbol=trait.symbol,
• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -316,9 +322,12 @@ def jsonable(trait, dataset=None): elif dataset.type == "Publish": if trait.pubmed_id: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

See above.

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -332,7 +341,10 @@ def jsonable(trait, dataset=None): ) else: return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)),

Ditto.

                     view=str(trait.view),

• symbol=trait_symbol, dataset=dataset.name, dataset_name=dataset.shortname, description=trait.description_display, @@ -346,6 +358,8 @@ def jsonable(trait, dataset=None): ) elif dataset.type == "Geno": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset=dataset.name, dataset_name=dataset.shortname, @@ -353,6 +367,8 @@ def jsonable(trait, dataset=None): ) elif dataset.name == "Temp": return dict(name=trait.name,
• display_name=trait.display_name,
• hmac=hmac.data_hmac('{}:{}'.format(trait.display_name, dataset.name)), view=str(trait.view), dataset="Temp", dataset_name="Temp")

Ditto (wrt to f-strings).

Worth noting, something that Arun pointed out elsewhere, when we move to python > 3.10, we should prefer pattern-matching with "match" over "if ... else" statements. See https://peps.python.org/pep-0636/ for more info.

[...]

From d5c6bb7abfb3535c1fa5d0d8df95645a213ab030 Mon Sep 17 00:00:00 2001 Subject: [PATCH 10/35] Remove HTML table from collections/view.html and replace with list of column definitions passed to create_datatable.js

---

.../wqflask/templates/collections/view.html | 215 +++++++++++------- 1 file changed, 138 insertions(+), 77 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6d0e3edd4..6ea129f96 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html

[...]

• {% endfor %}
• 
  Loading...
  

nitpick: [you can ignore this] "
" is much better than "
". No good reason why...

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'description')){
• try {
• return decodeURIComponent(escape(data.description))
• } catch(err){
• return escape(data.description)
• }
• } else {
• return "N/A"
• }
• }

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

[...]

• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'mean')){
• return data.mean.toFixed(3)
• } else {
• return "N/A"
• }
• }
• },

See above...

[...]

• 'title': "
  Peak
  LOD <a href=\"{{ url_for('glossary_blueprint.glossary') }}#LRS\" target=\"_blank\" style=\"color: white;\">^?
  ",

tip: nitpick: In cases like this, use single quotes to save yourself the chore of having to escape double-quotes.

• 'type': "natural-minus-na",
• 'data': null,
• 'width': "60px",
• 'targets': 8,
• 'orderSequence': [ "desc", "asc"],
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_score')){
• return (data.lrs_score / 4.61).toFixed(3)
• } else {
• return "N/A"
• }
• }

See above concern on extra args.

• {
• 'title': "
  Peak Location
  ",
• 'type': "natural-minus-na",
• 'data': null,
• 'width': "125px",
• 'targets': 9,
• 'render': function(data, type, row, meta) {
• if (Object.hasOwn(data, 'lrs_location')){
• return data.lrs_location
• } else {
• return "N/A"
• }
• }
• },

Ditto.

• {
• 'title': "
  Effect
  Size <a href=\"{{ url_for('glossary_blueprint.glossary') }}#A\" target=\"_blank\" style=\"color: white;\">^?
  ",

[...]

From 0ddd74a4d417389f7a35ac0971b0c17f5d79fb56 Mon Sep 17 00:00:00 2001 Subject: [PATCH 11/35] Change some variables in search_result_page.html to camelCase

+1 for this \m/\m/.

[...]

From ca368e32528073de15d925b4293b938f95f69dac Mon Sep 17 00:00:00 2001 Subject: [PATCH 12/35] Fix JS/CSS imports and include scroller setting in custom table settings

---

wqflask/wqflask/templates/collections/view.html | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 6ea129f96..a12c13695 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -3,3 +3,4 @@ {% block css %}

 <link rel="stylesheet" type="text/css" href="{{ url_for('js', filename='DataTablesExtensions/buttonStyles/css/buttons.dataTables.min.css') }}">

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

[...]

From 2e0e3aaa2c78c355e83cfae21c9655554451200b Mon Sep 17 00:00:00 2001 Subject: [PATCH 14/35] Fix manual column width change feature to work in create_datatable.js

---

.../static/new/javascript/create_datatable.js | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 7b495a8b7..217e99be6 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -2,9 +2,11 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

 loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

nitpick: prefer var over let. And this looks much nicer.

• // Store the change in width so overall table width can be adjusted by
• // that amount
• let widthChange = 0;

It's - sometimes - nice to have [literally] pretty code. It: (a) encourages contribs (b) Makes life easy when reviewing (c) Makes it easy when troubleshooting/debugging

nitpick: Of note, for comments, similar to git commits, using the imperative mood - sometimes - reads better. Also, try not to use append comments with your name. This usually tends to have a nasty effect on a person who would want to change something you wrote; something along the lines of 'I shouldn't touch this!'. Imagine if saw something like this sometime in the future:

// Munyoki: some comments follow...
let someConf = 2

What would you think if you were supposed to fiddle with that? At best, git blame allows you to find out who did what where.

[...]

From f22983a304d0c621ef3dab9aac8e824911e66485 Mon Sep 17 00:00:00 2001 Subject: [PATCH 15/35] Change JS variables to camelCase and assign ID to table container div

---

wqflask/wqflask/templates/collections/view.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index a12c13695..2349e16da 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

[...]

• create_table(table_id, traits_json, column_defs, table_settings)
• create_table(tableId, traitsJson, columnDefs, tableSettings)

+1 for using camelCase all over. Perhaps in this case, you do have a createTable too.

[...]

From 0ca935b8abda975334b89673ccef4bcd55837773 Mon Sep 17 00:00:00 2001 Subject: [PATCH 16/35] Added createdRow callback function to custom table settings for View Collection page

Also set a default table width of 2000px

.../wqflask/templates/collections/view.html | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/wqflask/wqflask/templates/collections/view.html b/wqflask/wqflask/templates/collections/view.html index 2349e16da..8dee52444 100644 --- a/wqflask/wqflask/templates/collections/view.html +++ b/wqflask/wqflask/templates/collections/view.html @@ -94,7 +94,7 @@

This collection has {{ '{}'.format(numify(trait_obs|count, "record", "record

Show/Hide Columns:

See above comment on putting things in CSS...

[...]

From 8a646b232b144323989f5cb701d18de4745920ba Mon Sep 17 00:00:00 2001 Subject: [PATCH 17/35] Fix issue where customSettings weren't being passed when manually adjusting column widths

---

wqflask/wqflask/static/new/javascript/create_datatable.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/wqflask/wqflask/static/new/javascript/create_datatable.js b/wqflask/wqflask/static/new/javascript/create_datatable.js index 217e99be6..2a895e185 100644 --- a/wqflask/wqflask/static/new/javascript/create_datatable.js +++ b/wqflask/wqflask/static/new/javascript/create_datatable.js @@ -1,10 +1,9 @@ create_table = function(tableId, tableData, columnDefs, customSettings) {

• loadDataTable(tableId=tableId, tableFata=tableData, customSettings=customSettings, firstRun=true)

• loadDataTable(tableId=tableId, tableData=tableData, customSettings, firstRun=true)

• var widthChange = 0; //ZS: For storing the change in width so overall table width can be adjusted by that amount

• var widthChange = 0; // For storing the change in width so overall table width can be adjusted by that amount function loadDataTable(tableId, tableData, customSettings, firstRun=false){

See above comment no var/let/comments.

[...]

From 714b64d8830cd5d4ca06cb07c162038155e56675 Mon Sep 17 00:00:00 2001 Subject: [PATCH 18/35] Change search_result_page.html to generate its table with create_datatable.js

---

.../wqflask/templates/search_result_page.html | 583 ++++++++---------- 1 file changed, 245 insertions(+), 338 deletions(-)

diff --git a/wqflask/wqflask/templates/search_result_page.html b/wqflask/wqflask/templates/search_result_page.html index fb7f404bf..0bce6793d 100644 --- a/wqflask/wqflask/templates/search_result_page.html +++ b/wqflask/wqflask/templates/search_result_page.html @@ -131,7 +131,7 @@ {% endif %}

[BonfaceKilz]

zsloan @.***> aliandika:

Thanks for the review! I appreciate you taking the time to look through all of this.

Sure.

I have no idea why the commented out code was still in initialize_show_trait_tables.js; must have just forgotten to delete that (for each file I would initially comment out the code I was trying to replace, and I must have just forgotten to remove it before committing with that file). Also, the comment with the "ZS" is from some older code copied over, so I'll just remove that;

Thanks!

I don't do that anymore with new comments. I'm not sure how to make a PR like this much smaller unless I just did a separate PR for each table, though; separating out stuff like converting variables to camelCase is a good idea (as you say) but most of these commits are still related to the actual table changes (and I frequently encountered issue that required me to go back and further edit create_datatable.js).

What I mean by making a PR small would be to have the things that do the superflous stuff, like camelCasing things, a separate PR. And for such changes, you can just merge it without anyone looking into it. Also, another thing you may find useful is: https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-with-git-commit-fixup-and-git-rebase-autosquash/

I already spoke with Rob about what to look for in testing (as you say, it's mostly just that functionality is the same).

Great!

I'll fix most of these things Monday, but a some answers to the questions (plus things I have questions about):

Sure. No pressure.

question: Curious, why do we pass a json_version when rendering the template? This to me is a red[-ish] flag. Is there an underlying reason to check for the json_version? IIRC, this was necessary years back; it shouldn't be the case in 2022.

Currently this is because the table loading looks bad if there are a very large number of traits in the collection and it has to load the whole table into the DOM. If passed as JSON, it only ever has to render a limited number of rows using Scroller, but if passed as HTML it has to render all of the rows before DataTables "converts" the table into one with its functionality (which can look weird with big tables, since it can take a second or two in those cases). There might be an alternate way to do this, but this is why it currently does it this way instead of just using a for-loop in the template to draw rows.

Ah I see. I reckon this is a classic case of naming bring confusion... json_version is a data-structure that has all the pre-processing done in the python end right? If that's the case, perhaps a better, less confusing name should suffice. I thought that "json_version" would be something like "1.0".

question: nitpick: Why are we tracking state .i.e. firstRun here? Does this mean that "loadDataTable" is run several times?

question: nitpick: This is an async recursive (?) call. Why?

This is because it runs any time columns manually have their width adjusted through clicking/dragging (since it needs to redraw the table). You can see where it's called in the "initComplete" table setting.

I see. Thanks for the clarification. When we get the bandwidth, we could re-think how this is done. For now, this makes sense and can fly.

We pass in tableSettings+tableData, and we use tableData to configure tableSettings. This to me seems like a case where you could pass a compound data structure.

Could you explain what you mean here?

Sure. FWIW, I've checked the sorrounding context, and indeed, this a non-issue. I had checked an earlier commit. Neveretheless, we had:

tableSettings = {
     'data': tableData,
     'columns': columnDefs,
     "sDom": "iti",
     "destroy": true,
     "autoWidth": false,
     "bSortClasses": false,
     "scrollY": "100vh",
     "scrollCollapse": true,
     "scroller":  true,
     "iDisplayLength": -1,
     "initComplete": function (settings) {
         // Add JQueryUI resizable functionality to each th in the ScrollHead table
         $('#' + table_id + '_wrapper .dataTables_scrollHead thead th').resizable({
             handles: "e",
             alsoResize: '#' + tableId + '_wrapper .dataTables_scrollHead table', //Not essential but makes the resizing smoother
             resize: function( event, ui ) {
                 widthChange = ui.size.width -

and the function signature was:

function loadDataTable(tableId, tableData, tableSettings, firstRun=false)

I thought that tableData+tableSettings could be combined into one data structure.

comment: nitpick: I strongly feel that "recheckRows" and "checkRows" could be the same thing. IIRC, the effort here is to reduce code duplication ;)

I don't think there's another function called checkRows. What are you referring to?

Again, misdirection with naming - and my faulty interpretation. While reading this, I thought there was a "recheckRows", there had to be a "checkRows". Naming can be hard. In this case, do you think it's a good idea to rename "recheckRows" to "checkRows"?

comment: nitpick: since this is a refactoring effort, perhaps a better name for "theTable" would suffice?

I agree, but I'm not sure what to call it. Just "table" feels too generic and not obviously a variable name at a glance. "thisTable" maybe, though it's kind of similar to "theTable"

The id in the relevant function signature:

function(tableId="trait_table", tableData = [], columnDefs = [], customSettings = {})

indicates that the table is used for loading trait data. Would traitsTable suffice?

comment: nitpick: firstRun and !firstRun are disjoint. That said, instead of "if(!firstRun)" and the later "if(firstRun)", you could lump this into an "if ... else" block.

Yeah, no idea why I didn't just put that inside the Else of the If/Else directly before it (assuming that's what you're referring to).

\m/\m/

question: Here, we only use "data" from the args. What's the point of having "type", "row" and "meta"?

There probably isn't one; I was just matching the syntax in the DataTables documentation, but I'll see if it works without all of those parameters passed (I'm guessing it should as long as I specify which one I'm passing).

Cool cool.

issue: Shouldn't we call this from Guix - our channel? The point of that channel is to fix all our js dependencies. Hard on the devs, but it guarantees that we are very picky with what we use... If this isn't packaged, please ping me/Alex/Fred to look into this. Nevertheless, for now let this stick, but let's not lose track of it.

You're correct; I added it like this for testing and just forgot to use the one in Guix (I'm pretty sure it actually is packaged, but if it isn't I'll let one of you know).

Cool. You could ping Alex and have him look into it - he'd do so after 5th August [and we can wait ;)]

This is a pattern I'm seeing in GN2, where we have in-line styles. In cases where we can dump things to CSS, let's do that.

In a situation where a styling is only applied to a single element, should I just add some extra class (like "search-table" or "correlation-table" or whatever -

Good question. I should think so. But in this case, don't create an extra class. Target it with CSS. The problem with inline CSS is that: (a) it takes first priority, there by making UI changes from CSS troublesome; (b) new contribs - myself included at one point - follow that pattern in cases where it isn't really required.

the problem is that the widths vary depending on the table, so there'd need to be a separate one for each type)?

Experiment with:

.my-target {
    width: calc(100% - 7px);
}

To make trouble-shooting easier, add a border. If that takes too much of your time, six-deep the idea and stick with the inline/JS CSS for now. We'd figure something out later.

Widths are also often conditional on something - should there just be separate class names for each condition in the CSS? (this also comes up in another comment below)

Ah I see. Then this is a good case for using JS to do the styling.

This: "{% if corr_method == 'pearson' %}r{% else %}rho{% endif %}" is repetitive. How about setting it once somewhere and using that value in the above snip.

question: Shouldn't we do this in Python?

These two are kind of pertaining to a similar thing. Regarding the first, I agree that it's probably best to just define "rOrRho" (or whatever) somewhere earlier and then use that variable instead of the jinja2 conditional, so I'll make that change.

Cool cool.

Regarding the latter (where you're commenting on the author_string being truncated with "et. al."), the reasoning I was using (which might be wrong) is to put things pertaining solely to the display in the template/JS. The alternative here would be to store a separate "authors_display" (or something) in the JSON (since I still want to pass the full authors list so it can be displayed when the user hovers their cursor over the cell). I think I'll go ahead and make this change (storing "authors_display" in the JSON row data), since if nothing else it's probably faster than doing it in the JS for every cell in the column.

Micro-optimisations :)

• 'title': "Description",
• 'type': "natural",
• {% if (max_widths.description * 7) < 500 %}
• 'width': "{{ max_widths.description * 7 }}px",
• {% else %}
• 'width': "500px",
• {% endif %}
• 'data': null,
• 'targets': 3,
• 'render': function(data, type, row, meta) {
• try {
• return decodeURIComponent(escape(data.description))
• } catch(err){
• return data.description
• } } See above comments on CSS + extra unused args.

I'm not sure how to deal with conditionals like this in the CSS, other than just having separate very specific class names (like "wide-description-column" and "short-description-column"; also, the column widths can vary on the specific table, so there'd either need to be separate CSS files for each of these tables or more variables variable names). Is this what you have in mind?

Here, my emphasis was on the extra args. With the CSS, don't stress yourself much with it... I was nitpicking.

-- (Life is like a pencil that will surely run out, but will leave the beautiful writing of life.) (D4F09EB110177E03C28E2FE1F5BBAE1E0392253F (hkp://keys.openpgp.org))