From 38272e8a6821a416907cf5e1bdc6bca80a8265ee Mon Sep 17 00:00:00 2001 From: Ken Sternberg <133134217+kensternberg-authentik@users.noreply.github.com> Date: Tue, 12 Dec 2023 03:04:39 -0800 Subject: [PATCH] web: refactor the table renderer for legibility (#7433) * web: break circular dependency between AKElement & Interface. This commit changes the way the root node of the web application shell is discovered by child components, such that the base class shared by both no longer results in a circular dependency between the two models. I've run this in isolation and have seen no failures of discovery; the identity token exists as soon as the Interface is constructed and is found by every item on the page. * web: fix broken typescript references This built... and then it didn't? Anyway, the current fix is to provide type information the AkInterface for the data that consumers require. * Refactor the Table component for legiibility. This commit does not change the functionality of the Table, nor does it require any changes to existing uses of the Table. It will probably be easier to review this by looking at the `View Code` in the upper-right-hand corner of GitHub's reviewer; that or side-by-side, if your monitor is wide-enough. The existing Table component is used 49 times (at last count) in authentik, and those uses are wide-ranging and complex, but they all come down to a couple of entries: - Displaying a row of summary information - Permitting the display of more complex ("expanded") information - Displaying a collection of rows - Displaying a collection of rows grouped by some header - Pagination of many rows - Permitting an action on the visible rows - *Not* blocking events that may happen on a cell or expansion - Providing a toolbar - Providing a display of "selected items" when using the table as a multi-select with many pages of items (chips display) - Providing sort functionality on columns - Providing the ability to filter the table from the back-end This commit changes none of that. What this commit does is re-arrange the innards of Table.ts into smaller units: - The RowGroup's "checkbox" and "expansion" segments are pulled out into their own functions, which makes the RowGroup's actual functionality much easier to see and understand. The same is true of the rowGroup's selection and expansion handlers. - Almost all in-line decisions and event handlers have been extracted and named, to make it easier to see and understand what's happening inside what is otherwise a jumble of HTML. - The TablePagination code was duplicated-- and one of the duplicates was wrong! So I've deduplicated it and fixed the bug. - In many cases, the conditional code grew organically, resulting in some pretty hard-to-understand conditions. - A really good example is the `itemSelectHandler`; there are two possible events that result in a change, and the consequences of that change may be that *all* checkboxes are unchecked. In all cases where there's an add/remove option, I've opted to remove the specific object always (even if it's not present!), and then add it if it's actually an add. Logically coherent as long as the accessors are not also mutators. It was not possible to redefine the `columns()` function to take anything other than a TableColumn object; I wanted to be able to replace all of the `new TableColumn("Foo")` with just `"Foo"`, building the TableColumn dynamically at construction time. Unfortunately, some of our most complex tables dynamically re-arrange the columns (RBAC, for example, draws an empty table, fetches the content, then redraws with the columns based on what was retrieved), and detecting that change and rebuilding those columns proved more difficult than anticipated. I may contemplate an alternative column specification if I find myself building a lot of tables. Likewise, it was not possible to replace all of our uses of the empty `html` declaration with the Lit-preferred `nothing` sigil; hard-coded `TemplateResult` entries scattered throughout the code caused massive type inconsistencies, since a type of `TemplateResult | nothing` is unique thanks to `nothing`'s underlying Symbol. It is for this issue that Typescript itself recommends you "prefer allowing Typescript infer the return type." I may revisit this issue later. I've added a `prequick` command to `package.json`; this one runs *only* the Typescript type checker, lit-analyse, and `eslint:precommit`, the last of which lints only the files touched since the last commit. This is fast, intended to support quick checks of code quality not normally displayed in the IDE. * web: refactor table After talking to Jens, I've put back the positional variable and eslint escape; it's better to document existing practices than try to force something. I also misunderstood the role of `inner` in one bit of code, and have restored its functionality. Looking through the code, though, I can see a case where it will fail; it's expecting `inner` to be either undefined or a TemplateResult; if there's no error message, the error message defaults to a blank TemplateResult, which is _not_ undefined, and will result in a blank table. This will only happen under very weird network failures, but... --- web/package.json | 1 + web/src/elements/table/Table.ts | 361 ++++++++++++++++---------------- 2 files changed, 177 insertions(+), 185 deletions(-) diff --git a/web/package.json b/web/package.json index 811e221ca..cecb3d40d 100644 --- a/web/package.json +++ b/web/package.json @@ -19,6 +19,7 @@ "lint:spelling": "codespell -D - -D ../.github/codespell-dictionary.txt -I ../.github/codespell-words.txt -S './src/locales/**' ./src -s", "lit-analyse": "lit-analyzer src", "precommit": "run-s tsc lit-analyse lint:precommit lint:spelling prettier", + "prequick": "run-s tsc:execute lit-analyse lint:precommit lint:spelling", "prettier-check": "prettier --check .", "prettier": "prettier --write .", "pseudolocalize:build-extract-script": "cd scripts && tsc --esModuleInterop --module es2020 --moduleResolution 'node' pseudolocalize.ts && mv pseudolocalize.js pseudolocalize.mjs", diff --git a/web/src/elements/table/Table.ts b/web/src/elements/table/Table.ts index 6175b5296..0b78c4dfb 100644 --- a/web/src/elements/table/Table.ts +++ b/web/src/elements/table/Table.ts @@ -13,6 +13,7 @@ import "@goauthentik/elements/table/TableSearch"; import { msg } from "@lit/localize"; import { CSSResult, TemplateResult, css, html } from "lit"; import { property, state } from "lit/decorators.js"; +import { classMap } from "lit/directives/class-map.js"; import { ifDefined } from "lit/directives/if-defined.js"; import PFButton from "@patternfly/patternfly/components/Button/button.css"; @@ -41,11 +42,7 @@ export class TableColumn { if (!this.orderBy) { return; } - if (table.order === this.orderBy) { - table.order = `-${this.orderBy}`; - } else { - table.order = this.orderBy; - } + table.order = table.order === this.orderBy ? `-${this.orderBy}` : this.orderBy; table.fetch(); } @@ -75,16 +72,12 @@ export class TableColumn { } render(table: Table): TemplateResult { - return html` + const classes = { + "pf-c-table__sort": !!this.orderBy, + "pf-m-selected": table.order === this.orderBy || table.order === `-${this.orderBy}`, + }; + + return html` ${this.orderBy ? this.renderSortable(table) : html`${this.title}`} `; } @@ -230,7 +223,7 @@ export abstract class Table extends AKElement { return html`
- +
`; @@ -241,11 +234,10 @@ export abstract class Table extends AKElement {
- ${inner - ? inner - : html`
${this.renderObjectCreate()}
- `} + ${inner ?? + html`
${this.renderObjectCreate()}
+ `}
@@ -257,14 +249,13 @@ export abstract class Table extends AKElement { } renderError(): TemplateResult { - if (!this.error) { - return html``; - } - return html` - ${this.error instanceof ResponseError - ? html`
${this.error.message}
` - : html`
${this.error.detail}
`} - `; + return this.error + ? html` + ${this.error instanceof ResponseError + ? html`
${this.error.message}
` + : html`
${this.error.detail}
`} + ` + : html``; } private renderRows(): TemplateResult[] | undefined { @@ -294,104 +285,89 @@ export abstract class Table extends AKElement { private renderRowGroup(items: T[]): TemplateResult[] { return items.map((item) => { const itemSelectHandler = (ev: InputEvent | PointerEvent) => { - let checked = false; const target = ev.target as HTMLElement; - if (ev.type === "input") { - checked = (target as HTMLInputElement).checked; - } else if (ev instanceof PointerEvent) { - if (target.classList.contains("ignore-click")) { - return; - } - checked = this.selectedElements.indexOf(item) === -1; - } - if (checked) { - // Prevent double-adding the element to selected items - if (this.selectedElements.indexOf(item) !== -1) { - return; - } - // Add item to selected - this.selectedElements.push(item); - } else { - // Get index of item and remove if selected - const index = this.selectedElements.indexOf(item); - if (index <= -1) return; - this.selectedElements.splice(index, 1); - } - this.requestUpdate(); - // Unset select-all if selectedElements is empty - const selectAllCheckbox = - this.shadowRoot?.querySelector("[name=select-all]"); - if (!selectAllCheckbox) { + if (ev instanceof PointerEvent && target.classList.contains("ignore-click")) { return; } - if (this.selectedElements.length < 1) { - selectAllCheckbox.checked = false; - this.requestUpdate(); + + const selected = this.selectedElements.includes(item); + const checked = + ev instanceof PointerEvent ? !selected : (target as HTMLInputElement).checked; + + if ((checked && selected) || !(checked || selected)) { + return; } + + this.selectedElements = this.selectedElements.filter((i) => i !== item); + if (checked) { + this.selectedElements.push(item); + } + + const selectAllCheckbox = + this.shadowRoot?.querySelector("[name=select-all]"); + if (selectAllCheckbox && this.selectedElements.length < 1) { + selectAllCheckbox.checked = false; + } + + this.requestUpdate(); }; - return html` + + const renderCheckbox = () => + html` + + `; + + const handleExpansion = (ev: Event) => { + ev.stopPropagation(); + const expanded = this.expandedElements.includes(item); + this.expandedElements = this.expandedElements.filter((i) => i !== item); + if (!expanded) { + this.expandedElements.push(item); + } + this.requestUpdate(); + }; + + const expandedClass = { + "pf-m-expanded": this.expandedElements.includes(item), + }; + + const renderExpansion = () => { + return html` + + `; + }; + + return html` - ${this.checkbox - ? html` - - ` - : html``} - ${this.expandable - ? html` - - ` - : html``} + ${this.checkbox ? renderCheckbox() : html``} + ${this.expandable ? renderExpansion() : html``} ${this.row(item).map((col) => { return html`${col}`; })} - + - ${this.expandedElements.indexOf(item) > -1 ? this.renderExpanded(item) : html``} + ${this.expandedElements.includes(item) ? this.renderExpanded(item) : html``} `; }); @@ -418,28 +394,24 @@ export abstract class Table extends AKElement { } renderSearch(): TemplateResult { - if (!this.searchEnabled()) { - return html``; - } - return html`
- { - this.search = value; - this.fetch(); - updateURLParams({ - search: value, - }); - }} - > - -
`; - } + const runSearch = (value: string) => { + this.search = value; + updateURLParams({ + search: value, + }); + this.fetch(); + }; - // eslint-disable-next-line @typescript-eslint/no-unused-vars - renderSelectedChip(item: T): TemplateResult { - return html``; + return !this.searchEnabled() + ? html`` + : html`
+ + +
`; } renderToolbarContainer(): TemplateResult { @@ -449,18 +421,7 @@ export abstract class Table extends AKElement {
${this.renderToolbar()}
${this.renderToolbarAfter()}
${this.renderToolbarSelected()}
- ${this.paginated - ? html` { - this.page = page; - updateURLParams({ tablePage: page }); - this.fetch(); - }} - > - ` - : html``} + ${this.paginated ? this.renderTablePagination() : html``} `; } @@ -469,57 +430,87 @@ export abstract class Table extends AKElement { this.fetch(); } + /* The checkbox on the table header row that allows the user to "activate all on this page," + * "deactivate all on this page" with a single click. + */ + renderAllOnThisPageCheckbox(): TemplateResult { + const checked = + this.selectedElements.length === this.data?.results.length && + this.selectedElements.length > 0; + + const onInput = (ev: InputEvent) => { + this.selectedElements = (ev.target as HTMLInputElement).checked + ? this.data?.results.slice(0) || [] + : []; + }; + + return html` + + `; + } + + /* For very large tables where the user is selecting a limited number of entries, we provide a + * chip-based subtable at the top that shows the list of selected entries. Long text result in + * ellipsized chips, which is sub-optimal. + */ + renderSelectedChip(_item: T): TemplateResult { + // Override this for chip-based displays + return html``; + } + + get needChipGroup() { + return this.checkbox && this.checkboxChip; + } + + renderChipGroup(): TemplateResult { + return html` + ${this.selectedElements.map((el) => { + return html`${this.renderSelectedChip(el)}`; + })} + `; + } + + /* A simple pagination display, shown at both the top and bottom of the page. */ + renderTablePagination(): TemplateResult { + const handler = (page: number) => { + updateURLParams({ tablePage: page }); + this.page = page; + this.fetch(); + }; + + return html` + + + `; + } + renderTable(): TemplateResult { - return html` ${this.checkbox && this.checkboxChip - ? html` - ${this.selectedElements.map((el) => { - return html`${this.renderSelectedChip(el)}`; - })} - ` - : html``} + const renderBottomPagination = () => + html`
${this.renderTablePagination()}
`; + + return html` ${this.needChipGroup ? this.renderChipGroup() : html``} ${this.renderToolbarContainer()} - ${this.checkbox - ? html`` - : html``} + ${this.checkbox ? this.renderAllOnThisPageCheckbox() : html``} ${this.expandable ? html`` : html``} ${this.columns().map((col) => col.render(this))} ${this.renderRows()}
- 0} - @input=${(ev: InputEvent) => { - if ((ev.target as HTMLInputElement).checked) { - this.selectedElements = - this.data?.results.slice(0) || []; - } else { - this.selectedElements = []; - } - }} - /> -
- ${this.paginated - ? html`
- { - this.page = page; - this.fetch(); - }} - > - -
` - : html``}`; + ${this.paginated ? renderBottomPagination() : html``}`; } render(): TemplateResult {