Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort Order Improvements for the Agents table #448

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 115 additions & 26 deletions server/src/static/assets/js/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,21 @@ document.addEventListener('DOMContentLoaded', function() {
});
});

function sortTable(header, table) {
var SORTABLE_STATES = {
none: 0,
ascending: -1,
descending: 1,
ORDER: ['none', 'ascending', 'descending'],
};

/**
* Sorts the specified table by the specified ordering
* @param {HTMLTableCellElement} header
* @param {HTMLTableElement} table
* @param {String} newOrder
*/
function sortTable(header, table, newOrder) {
// Get index of column based on position of header cell in <thead>
// We assume there is only one row in the table head.
var col = [].slice.call(table.tHead.rows[0].cells).indexOf(header);

// Based on the current aria-sort value, get the next state.
var newOrder = SORTABLE_STATES.ORDER.indexOf(header.getAttribute('aria-sort')) + 1;
newOrder = newOrder > SORTABLE_STATES.ORDER.length - 1 ? 0 : newOrder;
newOrder = SORTABLE_STATES.ORDER[newOrder];
const sortDirectionMap = {
none: 0,
ascending: -1,
descending: 1,
};

// Reset all header sorts.
var headerSorts = table.querySelectorAll('[aria-sort]');
Expand All @@ -43,7 +42,7 @@ function sortTable(header, table) {

// Get the direction of the sort and assume only one tbody.
// For this example only assume one tbody.
var direction = SORTABLE_STATES[newOrder];
var direction = sortDirectionMap[newOrder];
var body = table.tBodies[0];

// Convert the HTML element list to an array.
Expand All @@ -56,29 +55,92 @@ function sortTable(header, table) {
return a.getAttribute('data-index') - b.getAttribute('data-index');
});
} else {
sortOrderFunc = defaultSortOrder;
if (header.hasAttribute("use-outcome-sort")) {
sortOrderFunc = outcomeSortOrder;
}
// Sort based on a cell contents
newRows.sort(function (rowA, rowB) {
// Trim the cell contents.
var contentA = rowA.cells[col].textContent.trim();
var contentB = rowB.cells[col].textContent.trim();

// Based on the direction, do the sort.
//
// This example only sorts based on alphabetical order, to sort based on
// number value a more specific implementation would be needed, to provide
// number parsing and comparison function between text strings and numbers.
return contentA < contentB ? direction : -direction;
return sortOrderFunc(rowA.cells[col], rowB.cells[col], direction);
});
}
// Append each row into the table, replacing the current elements.
for (i = 0, ii = body.rows.length; i < ii; i += 1) {
body.appendChild(newRows[i]);
}

// Change url parameters based on sort order for the agents table
if (table.id == "agentsTable") {
var pageURL = new URL(window.location.href);
pageURL.searchParams.set("tableId", table.id);
pageURL.searchParams.set("headerId", header.id);
pageURL.searchParams.set("sortOrder", newOrder);
window.history.replaceState(null, null, pageURL);
}
}

/**
* Cycles through sort order states and applies the designated sort order to the
* specified table.
* @param {HTMLTableCellElement} header
* @param {HTMLTableElement} table
*/
function cycleTableSort(header, table) {
var SORTABLE_STATES = {
none: 0,
ascending: -1,
descending: 1,
ORDER: ['none', 'ascending', 'descending'],
};

// Based on the current aria-sort value, get the next state.
var newOrder = SORTABLE_STATES.ORDER.indexOf(header.getAttribute('aria-sort')) + 1;
newOrder = newOrder > SORTABLE_STATES.ORDER.length - 1 ? 0 : newOrder;
newOrder = SORTABLE_STATES.ORDER[newOrder];

sortTable(header, table, newOrder);
}

/**
* Default sort order for generic table columns. Uses an alphebetical ordering.
* @param {HTMLTableCellElement} cellA
* @param {HTMLTableCellElement} cellB
* @param {Number} direction
*/
function defaultSortOrder(cellA, cellB, direction) {
// Trim the cell contents.
var contentA = cellA.textContent.trim();
var contentB = cellB.textContent.trim();
return contentA < contentB ? direction : -direction;
}

/**
* Custom ordering function to sort the provisioning streak column in the agents table.
* Succeses and failures are grouped, then the streak number is always ordered
* in descending order.
* @param {HTMLTableCellElement} cellA
* @param {HTMLTableCellElement} cellB
* @param {Number} direction
*/
function outcomeSortOrder(cellA, cellB, direction) {
// Trim the provision streak number.
var streakA = cellA.getElementsByClassName("provision-streak")[0].textContent.trim();
var streakB = cellB.getElementsByClassName("provision-streak")[0].textContent.trim();
// Get success/failure icon of the row
var isFailureA = cellA.getElementsByClassName("p-icon--warning").length > 0;
var isFailureB = cellB.getElementsByClassName("p-icon--warning").length > 0;
// If both are failures or both are successes, we do a simple comparison
if (isFailureA == isFailureB) {
return parseInt(streakB) - parseInt(streakA);
}

// If A is failure and B is success, A should come before B in descending order
return isFailureA ? -direction : direction;
}

function setupClickableHeader(table, header) {
header.addEventListener('click', function () {
sortTable(header, table);
cycleTableSort(header, table);
});
}

Expand All @@ -102,9 +164,36 @@ function setupSortableTable(table) {
}
}

/**
* Gets URL parameters and uses them to sort the specified table.
* Currently only supports the agent table.
*/
function sortTableFromURL() {
// Get url parameters for table sort ordering if specified
const queryString = window.location.search;
const urlParams = new URLSearchParams(queryString);
const tableId = urlParams.get("tableId");
// Only url parameters for the agents table is supported
if (tableId != "agentsTable") {
return;
}
const headerId = urlParams.get("headerId");
var sortOrder = urlParams.get("sortOrder");
const validSortOrders = ["ascending", "descending", "none"];
if (!validSortOrders.includes(sortOrder)) {
sortOrder = "none"
}
var table = document.getElementById(tableId);
var header = document.getElementById(headerId);
if (table == null || header == null) {
return;
}
sortTable(header, table, sortOrder);
}

// Make all tables on the page sortable.
var tables = document.querySelectorAll('table');

for (var i = 0, ii = tables.length; i < ii; i += 1) {
setupSortableTable(tables[i]);
}
sortTableFromURL()
26 changes: 17 additions & 9 deletions server/src/templates/agents.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,18 @@ <h1 class="p-heading--3">
<button type="reset" class="p-search-box__reset"><i class="p-icon--close"></i></button>
<button type="submit" class="p-search-box__button"><i class="p-icon--search"></i></button>
</form>
<table aria-label="Agents table" class="p-table--mobile-card">
<table aria-label="Agents table" class="p-table--mobile-card" id="agentsTable">
<thead>
<tr>
<th aria-sort="none" class="has-overflow" style="width: 40pt"></th>
<!-- Added an empty header for the warning icon column -->
<th aria-sort="none">Name</th>
<th aria-sort="none">State</th>
<th aria-sort="none">Updated At</th>
<th aria-sort="none" class="has-overflow" style="width: 80pt" use-outcome-sort id="agentsOutcomeHeader">
Copy link
Contributor

@pedro-avalos pedro-avalos Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, great work, I have one comment in regards to the icons. You should add the p-table__cell--icon-placeholder on the th and td elements that have icons; this will make the icon display outside the column

See https://vanillaframework.io/docs/base/tables#icons

Suggested change
<th aria-sort="none" class="has-overflow" style="width: 80pt" use-outcome-sort id="agentsOutcomeHeader">
<th aria-sort="none" class="p-table__cell--icon-placeholder has-overflow" style="width: 80pt" use-outcome-sort id="agentsOutcomeHeader">

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The td element seems to be at line 38. I'm not sure how the icon placeholder behavior and the span and tooltip you have will interact with each other.

<td class="has-overflow">

Copy link
Contributor

@pedro-avalos pedro-avalos Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest you remove the style="width: 80pt on both the th and td elements, as it is generally bad practice to hard-code these styles into elements, and a hard-coded width can lead to some "fun" results...

image

<span class="p-tooltip--top-left" aria-describedby="tooltip">
<span class="p-tooltip__message" role="tooltip" id="tooltip">The number of consecutively successful / failed provisioning attempts</span>
Outcome
</span>
</th>
<th aria-sort="none" id="agentsNameHeader">Name</th>
<th aria-sort="none" id="agentsStateHeader">State</th>
<th aria-sort="none" id="agentsUpdatedAtHeader">Updated At</th>
<th>Job ID</th>
</tr>
</thead>
Expand All @@ -38,14 +42,18 @@ <h1 class="p-heading--3">
<span class="p-tooltip__message" role="tooltip" id="tooltip">This agent has failed the last {{
agent.provision_streak_count }} provision attempts</span>
</span>
{{ agent.provision_streak_count }}
<span class="provision-streak">
{{ agent.provision_streak_count }}
</span>
{% elif agent.provision_streak_type == "pass" and agent.provision_streak_count > 0 %}
<span class="p-tooltip--right" aria-describedby="tooltip">
<i class="p-icon--success"></i>
<span class="p-tooltip__message" role="tooltip" id="tooltip">This agent has passed the last {{
agent.provision_streak_count }} provision attempts</span>
</span>
{{ agent.provision_streak_count }}
<span class="provision-streak">
{{ agent.provision_streak_count }}
</span>
Comment on lines +45 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the indentation here?

{% endif %}
</td>
<td><a href="/agents/{{ agent.name }}">{{ agent.name }}</a></td>
Expand All @@ -56,4 +64,4 @@ <h1 class="p-heading--3">
{% endfor %}
</tbody>
</table>
{% endblock %}
{% endblock %}