trustchain-oc1-orchestral
/
authentik
Archived
10
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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...
This commit is contained in:
Ken Sternberg 2023-12-12 03:04:39 -08:00 committed by GitHub
parent 333d781f92
commit 38272e8a68
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 177 additions and 185 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
web/package.json
View File

@ -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",

301
web/src/elements/table/Table.ts
View File

@ -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<unknown>): TemplateResult {
return html`<th
role="columnheader"
scope="col"
class="
${this.orderBy ? "pf-c-table__sort " : " "}
${table.order === this.orderBy || table.order === `-${this.orderBy}`
? "pf-m-selected "
: ""}
"
>
const classes = {
"pf-c-table__sort": !!this.orderBy,
"pf-m-selected": table.order === this.orderBy || table.order === `-${this.orderBy}`,
};
return html`<th role="columnheader" scope="col" class="${classMap(classes)}">
${this.orderBy ? this.renderSortable(table) : html`${this.title}`}
</th>`;
}
@ -230,7 +223,7 @@ export abstract class Table<T> extends AKElement {
return html`<tr role="row">
<td role="cell" colspan="25">
<div class="pf-l-bullseye">
<ak-empty-state ?loading="${true}" header=${msg("Loading")}> </ak-empty-state>
<ak-empty-state loading header=${msg("Loading")}> </ak-empty-state>
</div>
</td>
</tr>`;
@ -241,9 +234,8 @@ export abstract class Table<T> extends AKElement {
<tr role="row">
<td role="cell" colspan="8">
<div class="pf-l-bullseye">
${inner
? inner
: html`<ak-empty-state header="${msg("No objects found.")}"
${inner ??
html`<ak-empty-state header="${msg("No objects found.")}"
><div slot="primary">${this.renderObjectCreate()}</div>
</ak-empty-state>`}
</div>
@ -257,14 +249,13 @@ export abstract class Table<T> extends AKElement {
}
renderError(): TemplateResult {
if (!this.error) {
return html``;
}
return html`<ak-empty-state header="${msg("Failed to fetch objects.")}" icon="fa-times">
return this.error
? html`<ak-empty-state header="${msg("Failed to fetch objects.")}" icon="fa-times">
${this.error instanceof ResponseError
? html` <div slot="body">${this.error.message}</div> `
: html`<div slot="body">${this.error.detail}</div>`}
</ak-empty-state>`;
</ak-empty-state>`
: html``;
}
private renderRows(): TemplateResult[] | undefined {
@ -294,104 +285,89 @@ export abstract class Table<T> 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")) {
if (ev instanceof PointerEvent && target.classList.contains("ignore-click")) {
return;
}
checked = this.selectedElements.indexOf(item) === -1;
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) {
// 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<HTMLInputElement>("[name=select-all]");
if (!selectAllCheckbox) {
return;
}
if (this.selectedElements.length < 1) {
if (selectAllCheckbox && this.selectedElements.length < 1) {
selectAllCheckbox.checked = false;
this.requestUpdate();
}
this.requestUpdate();
};
return html`<tbody
role="rowgroup"
class="${this.expandedElements.indexOf(item) > -1 ? "pf-m-expanded" : ""}"
>
<tr
role="row"
class="${this.checkbox ? "pf-m-hoverable" : ""}"
@click=${itemSelectHandler}
>
${this.checkbox
? html`<td class="pf-c-table__check" role="cell">
const renderCheckbox = () =>
html`<td class="pf-c-table__check" role="cell">
<label class="ignore-click"
><input
type="checkbox"
class="ignore-click"
.checked=${this.selectedElements.indexOf(item) >= 0}
.checked=${this.selectedElements.includes(item)}
@input=${itemSelectHandler}
@click=${(ev: Event) => {
ev.stopPropagation();
}}
/></label>
</td>`
: html``}
${this.expandable
? html`<td class="pf-c-table__toggle" role="cell">
<button
class="pf-c-button pf-m-plain ${this.expandedElements.indexOf(
item,
) > -1
? "pf-m-expanded"
: ""}"
@click=${(ev: Event) => {
</td>`;
const handleExpansion = (ev: Event) => {
ev.stopPropagation();
const idx = this.expandedElements.indexOf(item);
if (idx <= -1) {
// Element is not expanded, add it
const expanded = this.expandedElements.includes(item);
this.expandedElements = this.expandedElements.filter((i) => i !== item);
if (!expanded) {
this.expandedElements.push(item);
} else {
// Element is expanded, remove it
this.expandedElements.splice(idx, 1);
}
this.requestUpdate();
}}
};
const expandedClass = {
"pf-m-expanded": this.expandedElements.includes(item),
};
const renderExpansion = () => {
return html`<td class="pf-c-table__toggle" role="cell">
<button
class="pf-c-button pf-m-plain ${classMap(expandedClass)}"
@click=${handleExpansion}
>
<div class="pf-c-table__toggle-icon">
&nbsp;<i class="fas fa-angle-down" aria-hidden="true"></i
>&nbsp;
&nbsp;<i class="fas fa-angle-down" aria-hidden="true"></i>&nbsp;
</div>
</button>
</td>`
: html``}
</td>`;
};
return html`<tbody role="rowgroup" class="${classMap(expandedClass)}">
<tr
role="row"
class="${this.checkbox ? "pf-m-hoverable" : ""}"
@click=${itemSelectHandler}
>
${this.checkbox ? renderCheckbox() : html``}
${this.expandable ? renderExpansion() : html``}
${this.row(item).map((col) => {
return html`<td role="cell">${col}</td>`;
})}
</tr>
<tr
class="pf-c-table__expandable-row ${this.expandedElements.indexOf(item) > -1
? "pf-m-expanded"
: ""}"
role="row"
>
<tr class="pf-c-table__expandable-row ${classMap(expandedClass)}" role="row">
<td></td>
${this.expandedElements.indexOf(item) > -1 ? this.renderExpanded(item) : html``}
${this.expandedElements.includes(item) ? this.renderExpanded(item) : html``}
</tr>
</tbody>`;
});
@ -418,30 +394,26 @@ export abstract class Table<T> extends AKElement {
}
renderSearch(): TemplateResult {
if (!this.searchEnabled()) {
return html``;
}
return html`<div class="pf-c-toolbar__group pf-m-search-filter">
<ak-table-search
class="pf-c-toolbar__item pf-m-search-filter"
value=${ifDefined(this.search)}
.onSearch=${(value: string) => {
const runSearch = (value: string) => {
this.search = value;
this.fetch();
updateURLParams({
search: value,
});
}}
this.fetch();
};
return !this.searchEnabled()
? html``
: html`<div class="pf-c-toolbar__group pf-m-search-filter">
<ak-table-search
class="pf-c-toolbar__item pf-m-search-filter"
value=${ifDefined(this.search)}
.onSearch=${runSearch}
>
</ak-table-search>
</div>`;
}
// eslint-disable-next-line @typescript-eslint/no-unused-vars
renderSelectedChip(item: T): TemplateResult {
return html``;
}
renderToolbarContainer(): TemplateResult {
return html`<div class="pf-c-toolbar">
<div class="pf-c-toolbar__content">
@ -449,18 +421,7 @@ export abstract class Table<T> extends AKElement {
<div class="pf-c-toolbar__bulk-select">${this.renderToolbar()}</div>
<div class="pf-c-toolbar__group">${this.renderToolbarAfter()}</div>
<div class="pf-c-toolbar__group">${this.renderToolbarSelected()}</div>
${this.paginated
? html`<ak-table-pagination
class="pf-c-toolbar__item pf-m-pagination"
.pages=${this.data?.pagination}
.pageChangeHandler=${(page: number) => {
this.page = page;
updateURLParams({ tablePage: page });
this.fetch();
}}
>
</ak-table-pagination>`
: html``}
${this.paginated ? this.renderTablePagination() : html``}
</div>
</div>`;
}
@ -469,57 +430,87 @@ export abstract class Table<T> extends AKElement {
this.fetch();
}
renderTable(): TemplateResult {
return html` ${this.checkbox && this.checkboxChip
? html`<ak-chip-group>
${this.selectedElements.map((el) => {
return html`<ak-chip>${this.renderSelectedChip(el)}</ak-chip>`;
})}
</ak-chip-group>`
: html``}
${this.renderToolbarContainer()}
<table class="pf-c-table pf-m-compact pf-m-grid-md pf-m-expandable">
<thead>
<tr role="row">
${this.checkbox
? html`<td class="pf-c-table__check" role="cell">
/* 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`<td class="pf-c-table__check" role="cell">
<input
name="select-all"
type="checkbox"
aria-label=${msg("Select all rows")}
.checked=${this.selectedElements.length ===
this.data?.results.length &&
this.selectedElements.length > 0}
@input=${(ev: InputEvent) => {
if ((ev.target as HTMLInputElement).checked) {
this.selectedElements =
this.data?.results.slice(0) || [];
} else {
this.selectedElements = [];
}
}}
.checked=${checked}
@input=${onInput}
/>
</td>`
: html``}
</td>`;
}
/* 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`<ak-chip-group>
${this.selectedElements.map((el) => {
return html`<ak-chip>${this.renderSelectedChip(el)}</ak-chip>`;
})}
</ak-chip-group>`;
}
/* 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`
<ak-table-pagination
class="pf-c-toolbar__item pf-m-pagination"
.pages=${this.data?.pagination}
.pageChangeHandler=${handler}
>
</ak-table-pagination>
`;
}
renderTable(): TemplateResult {
const renderBottomPagination = () =>
html`<div class="pf-c-pagination pf-m-bottom">${this.renderTablePagination()}</div>`;
return html` ${this.needChipGroup ? this.renderChipGroup() : html``}
${this.renderToolbarContainer()}
<table class="pf-c-table pf-m-compact pf-m-grid-md pf-m-expandable">
<thead>
<tr role="row">
${this.checkbox ? this.renderAllOnThisPageCheckbox() : html``}
${this.expandable ? html`<td role="cell"></td>` : html``}
${this.columns().map((col) => col.render(this))}
</tr>
</thead>
${this.renderRows()}
</table>
${this.paginated
? html` <div class="pf-c-pagination pf-m-bottom">
<ak-table-pagination
class="pf-c-toolbar__item pf-m-pagination"
.pages=${this.data?.pagination}
.pageChangeHandler=${(page: number) => {
this.page = page;
this.fetch();
}}
>
</ak-table-pagination>
</div>`
: html``}`;
${this.paginated ? renderBottomPagination() : html``}`;
}
render(): TemplateResult {