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

Add table sorting #1152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add table sorting #1152

wants to merge 1 commit into from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 20, 2024

Fixes #1120.

}

button.onclick = () => {
const isAscending = header.getAttribute("aria-sort") !== "ascending";
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty confusing assignment. I think that your intent is to say that the sort should be ascending when the current state is not ascending, but the negation and naming here makes it look backwards. Maybe:

Suggested change
const isAscending = header.getAttribute("aria-sort") !== "ascending";
const sortAscending = header.getAttribute("aria-sort") !== "ascending";

Comment on lines +499 to +501
return ascending
? a.localeCompare(b, undefined, { numeric: sortNumerically })
: b.localeCompare(a, undefined, { numeric: sortNumerically });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return ascending
? a.localeCompare(b, undefined, { numeric: sortNumerically })
: b.localeCompare(a, undefined, { numeric: sortNumerically });
[a, b] = ascending ? [a, b] : [b, a];
return a.localeCompare(b, undefined, { numeric: sortNumerically });

Comment on lines +468 to +470
if (isInitDescending) {
header.setAttribute("aria-sort", "descending");
}
Copy link
Member

Choose a reason for hiding this comment

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

From the code, it wasn't clear to me that this column was:
a) reverse-sorted to begin with
b) effectively the primary sort column for the table

A comment to that effect might help.

}
const button = document.createElement("button");
const isInitDescending = header.classList.contains("more-info");
const sortNumerically = isInitDescending;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const sortNumerically = isInitDescending;
const sortNumerically = isInitDescending; // Coincidentally, this is the only numeric column.

return;
}
const button = document.createElement("button");
const isInitDescending = header.classList.contains("more-info");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isInitDescending = header.classList.contains("more-info");
// "more-info" is the column that starts out being sorted; in reverse order.
const isInitDescending = header.classList.contains("more-info");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort entries on the dashboard
2 participants