Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### 🚨 Breaking changes

### ✨ New features and improvements
- Added row numbers to tables using React Table v8, allowing users to identify row positions after sorting and filtering (#8089)
- Allowed instructors assigned as graders to switch between all submissions and only their assigned submissions in the submissions, summary, and grading views (#8083)
- Improved Session Timeout Logic: `check_timeout` polling paused when user is not focused on the MarkUs tab and polling stops after user session has timed out (#8074)
- Migrated Groups Manager students and groups tables to use `react-table` v8 (#8068)
Expand Down
21 changes: 21 additions & 0 deletions app/assets/stylesheets/common/_table.scss
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
flex: 99999 1 auto;
display: flex;
flex-direction: column;
counter-reset: table-row-number;
overflow: auto;
border-bottom: 1px solid $gridline;
border-left: 1px solid $gridline;
Expand All @@ -190,6 +191,24 @@
}
}

.rt-tr {
counter-increment: table-row-number;

&::before {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In terms of styling, please have this be the same background colour as the header cells. Also please apply this background colour to the full header rows as well (so that the empty top-left corners still appear have the background colours applied.

align-items: center;
bottom: 0;
content: counter(table-row-number);
display: flex;
font-size: 0.85em;
font-variant-numeric: tabular-nums;
justify-content: center;
left: 0;
position: absolute;
top: 0;
width: var(--row-number-gutter-width);
}
}

.rt-expandable {
cursor: pointer;
text-overflow: clip;
Expand All @@ -210,6 +229,8 @@
.rt-tr {
flex: 1 0 auto;
display: inline-flex;
padding-left: var(--row-number-gutter-width);
position: relative;
}

.rt-th,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe("For the InstructorTable's display of instructors", () => {
const instructors_in_one_row = instructor => {
const rows = screen.getAllByRole("row");
for (let row of rows) {
const cells = Array.from(row.childNodes).map(c => c.textContent);
const cells = Array.from(row.querySelectorAll(".rt-td"), c => c.textContent);
if (cells[0] === instructor.user_name) {
expect(cells[1]).toEqual(instructor.first_name);
expect(cells[2]).toEqual(instructor.last_name);
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/Components/__tests__/student_table.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe("For the StudentTable's display of students", () => {
const student_in_one_row = student => {
const rows = screen.getAllByRole("row");
for (let row of rows) {
const cells = Array.from(row.childNodes).map(c => c.textContent);
const cells = Array.from(row.querySelectorAll(".rt-td"), c => c.textContent);
if (cells[1] === student.user_name) {
expect(cells[2]).toEqual(student.first_name);
expect(cells[3]).toEqual(student.last_name);
Expand Down
30 changes: 29 additions & 1 deletion app/javascript/Components/__tests__/table.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,20 @@ describe("tests for the table component", () => {

expectRowsInTableInOrder(table, columns, data);
});

it("shows positional row numbers without adding a table column", () => {
const {table} = renderTableWithMockData();
const tableElement = table.querySelector(".Table");
const header = table.querySelector(".rt-thead.-header");
const tableRows = table.querySelectorAll(".rt-tbody .rt-tr");

expect(tableElement.style.getPropertyValue("--row-number-gutter-width")).toBe("40px");
expect(header.querySelectorAll(".rt-th")).toHaveLength(mockColumns().length);
tableRows.forEach(tableRow => {
expect(tableRow.querySelectorAll(".rt-td")).toHaveLength(mockColumns().length);
expect(tableRow.querySelector(".rt-row-number")).not.toBeInTheDocument();
});
});
});

describe("rendering of the no-data component when data is empty", () => {
Expand Down Expand Up @@ -226,6 +240,19 @@ describe("tests for the table component", () => {
expectRowsInTableInOrder(table, columns, data);
}
});

it("sorts row contents without adding a row-number column", async () => {
const {table, columns, data} = renderTableWithMockData();

await clickHeader(columns[0].header);
await clickHeader(columns[0].header);

const tableRows = table.querySelectorAll(".rt-tbody .rt-tr");
expect(within(tableRows[0]).getByText(data[data.length - 1].col1)).toBeInTheDocument();
tableRows.forEach(tableRow =>
expect(tableRow.querySelectorAll(".rt-td")).toHaveLength(columns.length)
);
});
});

describe("filtering", () => {
Expand All @@ -246,7 +273,7 @@ describe("tests for the table component", () => {
});

it("filters data", async () => {
const {columns} = renderTableWithMockData();
const {table, columns} = renderTableWithMockData();
columns.pop();

const inputs = searchInputs();
Expand All @@ -266,6 +293,7 @@ describe("tests for the table component", () => {
],
columns
);
expect(table.querySelectorAll(".rt-tbody .rt-tr")).toHaveLength(2);
});

it("resets the filter when the search query is erased", async () => {
Expand Down
23 changes: 16 additions & 7 deletions app/javascript/Components/table/table.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const defaultNoDataText = () => I18n.t("table.no_data");
const columnHelper = createColumnHelper();
export const SELECTION_COLUMN_ID = "select";
const FILTER_VARIANT_SELECT = "select";
const ROW_NUMBER_GUTTER_WIDTH = 40;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Define this as a CSS variable in _constants.scss. Also let's reduce the size to 30.


export const expanderColumn = columnHelper.display({
id: "expander",
Expand Down Expand Up @@ -173,9 +174,11 @@ export default function Table({
});

const centerTotalSize = table.getCenterTotalSize();
const tableTotalSize = centerTotalSize + ROW_NUMBER_GUTTER_WIDTH;
const rows = table.getRowModel().rows;

const tableHeaders = (
<div className="rt-thead -header" style={{minWidth: centerTotalSize}}>
<div className="rt-thead -header" style={{minWidth: tableTotalSize}}>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall this is great, but looking through the tables on various pages I'm not sure we should apply this universally. Can you please add a showRowNumbers prop to the Table component to control conditionally showing these row numbers? And then make the default false, so that it's only true for the assignment summary grades table.

{table.getHeaderGroups().map(headerGroup => (
<div className="rt-tr" role="row" key={headerGroup.id}>
{headerGroup.headers.map(header => (
Expand All @@ -200,7 +203,7 @@ export default function Table({
[table, finalColumns]
);
const tableFilters = showFilters && (
<div className="rt-thead -filters" style={{minWidth: centerTotalSize}}>
<div className="rt-thead -filters" style={{minWidth: tableTotalSize}}>
{table.getHeaderGroups().map(headerGroup => (
<div className="rt-tr" role="row" key={headerGroup.id}>
{headerGroup.headers.map(header => (
Expand All @@ -222,12 +225,18 @@ export default function Table({
);

return (
<div className="Table -highlight" style={{maxHeight: "500px"}}>
<div
className="Table -highlight"
style={{
maxHeight: "500px",
"--row-number-gutter-width": `${ROW_NUMBER_GUTTER_WIDTH}px`,
}}
>
<div className="rt-table" role="grid">
{tableHeaders}
{tableFilters}
<div className="rt-tbody" style={{minWidth: centerTotalSize}}>
{table.getRowModel().rows.map(row => (
<div className="rt-tbody" style={{minWidth: tableTotalSize}}>
{rows.map(row => (
<TableRow
row={row}
isExpanded={row.getIsExpanded()}
Expand All @@ -241,7 +250,7 @@ export default function Table({
columns={finalColumns}
/>
))}
{loading && table.getRowModel().rows.length > 0 && (
{loading && rows.length > 0 && (
<div
className="loading-spinner"
style={{
Expand All @@ -267,7 +276,7 @@ export default function Table({
/>
</div>
)}
{!table.getRowModel().rows.length &&
{!rows.length &&
(loading ? (
<div className="loading-spinner">
<Grid
Expand Down