0
\$\begingroup\$

I created a quick "demonstration of proposal behavior" tool for my post on MSO: A proposal to put ALL answerers on a path to curating better content.

Because I have never asked for a review before and I am not a JavaScript or CSS SME, I'd like to see if I've used any antipatterns and how my code can be made more direct/maintainable/readable/professional.

The technique that I used to create repeatable table rows doesn't feel very slick.

I don't necessarily need the actual presentation to be improved (since it was just something I scratched up), but if anyone want to go down that rabbit hole, I won't stop you.

$(document).on('click', '.add', function() {
  let mostRecentAnswer = $('#summary').prev(),
      id = !mostRecentAnswer.length ? 1 : 1 + mostRecentAnswer.data('id');
  $('#summary').before(
    '<tr data-id=' + id + '>'
      + '<td><input type="button" class="del" value="-"></td>'
      + '<td>'
        + '<label for="open' + id + '"><input type="radio" id="open' + id + '" name="status' + id + '" checked> Open</label>'
        + '<label for="closed' + id + '"><input type="radio" id="closed' + id + '" name="status' + id + '"> Closed</label>'
      + '</td>'
      + '<td class="newest5 hide"></td>'
      + '<td class="newest10 hide"></td>'
      + '<td class="newest20 hide"></td>'
    + '</tr>'
  );
  handleNewest();
});

$(document).on('click', '.del', function() {
  $(this).closest('tr').remove();
  handleNewest();
});

$(document).on('change', '[type="radio"]', function() {
  handleNewest();
});

function handleNewest() {
  let totalAnswers = $('#demo tr[data-id]').length;
  if (totalAnswers < 5) {
    $('#outcome').html('<b class="red">Please post carefully constructed and educational answers to questions which are at least 4 hours old -- it is presumed that this community has had ample to time to vet these new questions as clear, complete, unique and on-topic.</b>');
    return;
  }
  $('#outcome').html('More than 40% of your recent answers have been on open questions which the community has deemed to be a good fit for our repository of knowledge.');
  [5, 10, 20].forEach(function(group) {
    $('td[class^="newest' + group + '"]').addClass('hide').removeClass('groupStart groupEnd');
    if (totalAnswers >= group) {
      let lastCount = group - 1,
          sumOpen = 0,
          row = $('#summary').prev(),
          groupCell;
      for (let i = 0; i < group; ++i) {
        sumOpen += $('td label input[id^="open"]:checked', row).length;
        groupCell = $('td[class^="newest' + group + '"]', row);
        groupCell.removeClass('hide');
        if (!i) {
          groupCell.addClass('groupEnd');
        } else if (i === lastCount) {
          groupCell.addClass('groupStart');
        }
        row = row.prev();
      }
      let percent = Math.round(sumOpen / group * 100);
      $('#newest' + group + 'Total').html(percent).toggleClass('red', percent <= 40);
        $('td[class^="newest' + group + '"]:not(.hide)').toggleClass('redBG', percent <= 40);
        if (percent <= 40) {
          $('#outcome').html('<b  class="red">' + (100 - percent) + '% of your last ' + group + ' answers have been on closed pages. Please take more care to only post answers to clear, complete, unique, on-topic questions.</b>');
        }
      }
    });
  }
#demo {
  border-spacing: 0;
}

.newest5:not(.hide),
.newest10:not(.hide),
.newest20:not(.hide) {
  border-right: 1px solid black; min-width: 30px;
}

.groupStart {
  border-top-right-radius: 30px;
}

.groupEnd {
  border-bottom-right-radius: 30px;
}

#summaryLabel {
  text-align: right;
}

#[id$="Total"]:after {
  content: '%';
}

.red {
  color: red;
}

.newest5.redBG:not(.hide),
.newest10.redBG:not(.hide),
.newest20.redBG:not(.hide) {
  background-color: red;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<table id="demo">
  <tr id="summary">
    <td><input type="button" class="add" value="+"></td>
    <td id="summaryLabel">Open Percent:</td>
    <td id="newest5Total"></td>
    <td id="newest10Total"></td>
    <td id="newest20Total"></td>
  </tr>
</table>
<h2 id="outcome"></h2>

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Declarations

Always declare variables that do not change as const rather than let or var

For example the line

let mostRecentAnswer = $('#summary').prev(),
      id = !mostRecentAnswer.length ? 1 : 1 + mostRecentAnswer.data('id');

Should start with const and if written using native API

const id = Number(summary.previousSibling?.dataset.id ?? 0) + 1;

References ?? (Nullish coalescing operator), ?. (Optional chaining), Node.previousSibling, HTMLElement.dataset and Number

Note that summary must reference a unique Element via its unique Element.id. Unique means unique on page (including Global Scope)

Markup

Avoid adding markup directly to the page. jQuery users have a tendency to manipulate page content via markup strings.

Most IDE will not provide markup syntax styling when the markup is bound by quotes inside a script, significantly decreasing the readability and maintainability of a major part of your content.

Most importantly is that adding and parsing markup is up to 2 orders of magnitude slower than using native APIs.

Table via Native API

In your code you are building a HTMLTableElement. The table element provides a good set of properties and functions to quickly and easily manipulate rows, columns, and cells. HTMLTableElement.rows, HTMLTableRowElement, and HTMLTableRowElement to link but a few.

To create elements you can us Document.createElement using Object.assign to assign properties.

Example util functions to help build page content

const tag = (type, props = {}) => Object.assign(document.createElement(type), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par));
const row = (table, props, ...cells) => 
    const row = Object.assign(table.insertRow(-1), props);
    cells.map(cell => append(Object.assign(row.insertCell(-1), cell.shift()), ...cell));
    return row;
}

Example using native API to add and remove rows from a table

;(()=>{
"use strict";

// Global DOM utils. Note each call returns an element as defined by first arg
const tag = (type, props = {}) => Object.assign(document.createElement(type), props);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);
const event = (element, type, call, opts = {}) => (element.addEventListener(type, call, opts), element);

// return new row
const row = (table, props, ...cells) => {
    const row = Object.assign(table.insertRow(-1), props);
    cells.map(cell => append(Object.assign(row.insertCell(-1), cell.shift()), ...cell));
    return row;
};

// App code
const newCellProp = num => ({className: "newest" + num + " hide"});
const table = tag("table");
append(document.body,
    table, event(tag("button", {textContent: "AddRow"}), "click", addRow)
);
var id = 1;
function addRow() {
    const name = "Status" + id;
    const removeBtn = event(
        tag("input", {type: "button", className: "del", value: "-"}),
        "click",
        () => table.deleteRow([...table.rows].indexOf(newRow)));
    
    const newRow = row(
        table, {}, [{}, removeBtn], [{}, tag("span", {textContent: name})], [{},
            append(
                tag("label", {for: "open" + id, textContent: "Open"}),
                tag("input", {type: "radio", id: "open" + id, checked: true, textContent: " Open", name})
            ),
            append(
                tag("label", {for: "close" + id, textContent: "Close"}),
                tag("input", {type: "radio", id: "close" + id, textContent: " Closed", name})
            ),
        ], [newCellProp(5)], [newCellProp(10)], [newCellProp(15)]);
    newRow.dataset.id = id++;
}
})();

Use a template

Alternatively you can use a HTMLTemplateElement to define the table and contained elements in the page. The template element is not visible, You copy parts from it to create new page content.

jQuery

Your code is too reliant on jQuery. It is my opinion that jQuery is dead and one should avoid its use.

Reasoning...

Browser support

The most common reason people give for still using jQuery is legacy browser support.

I agree that jQuery is a great tool if you wish to support the very few that still use Internet Explorer (IE).

However this argument fails when your JavaScript is not written with legacy browsers in mind.

In your case you use let which has no support on IE6-10 and partial support on IE11. Can I use let

To use jQuery and still use modern JS (ECMAScript2015+) one must also include a transcompiler (for example BabelJS) to ensure full support

Performance

HTML5 provides very good coverage and using native API's reduce page load times (no need to load and parse bloated jQuery script) and significantly increase execution speed. If you need to also bundle a transcompiler (eg BabelJS) the page load will further be slowed.

Skills

You as a front end developer using jQuery do not gain experience using the native API's an important current and future skill set to master.

Verbose native API

Some will argue that jQuery is less verbose than the native APIs, and this is true, however in less than a dozen lines one can create functions to help call the most common DOM related tasks and greatly reduce the verbosity overhead associated with native API call.

For example. You can replace the long Element.querySelector and Element.querySelectorAll with 2 functions

const query = (qStr, element = document) => element.querySelector(qStr);
const queryAll = (qStr, element = document) => [...element.querySelectorAll(qStr)];

Also see example code in previous section.


Answer Note

Sorry I have run out of time so could not complete this review. Rather than drop it all, I applied "Something is better than nothing" as there is currently no answer to your post. Hope this helps.

\$\endgroup\$
4
  • \$\begingroup\$ The summary id is unique on the document. Why do you mention this? Good review, thanks. \$\endgroup\$ Aug 18, 2021 at 3:54
  • \$\begingroup\$ @mickmackusa All Element.id must be unique or the page is not valid. When using the Element.id as a name to reference an element eg <div id="byIdName"></div><script>console.log(byIdName === document.getElementById("byIdName")) // >> true</script> That id must be unique on both the HTML and the global JS scope. Declaring the name in JS eg var byIdName will remove the reference, within that scope, to the element. Using the id twice in the HTML makes the JS reference undefined (well sort of as there are many edge cases, and browser type and version caveats) \$\endgroup\$
    – Blindman67
    Aug 18, 2021 at 8:21
  • \$\begingroup\$ I am not confused about the rule. I am confused about why you are telling me in the review that my id must be unique. Is this just additional fact sharing? Because I don't see in my code where I have violated the rule. \$\endgroup\$ Aug 18, 2021 at 10:13
  • 1
    \$\begingroup\$ @mickmackusa Though you do not have the same ID on the page at any one time, you do re-assign the same id when you add a new row after deleting rows. Thought the browser will re-assign the id name with the reference to the new element when the element is added to the page, the reference becomes undefined when the element is created and before it has been added. This is not an issue when adding content via markup, it can be problematic when building content via the DOM API's \$\endgroup\$
    – Blindman67
    Aug 18, 2021 at 18:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.