From 3b4530fb7f2685855bb5c5b0c339b6583018caee Mon Sep 17 00:00:00 2001 From: Ken Sternberg Date: Fri, 1 Sep 2023 14:58:33 -0700 Subject: [PATCH] web: Revised navigation After working with the navigation for awhile, I realized that it's a poor map; what I really wanted was a controller/view pair, where events flow up to the controller and then messages on "what to draw" flow down to the view. It work quite well, and the wizard frame is smaller and smarter for it. I've also moved the WDIO-driven tests into the 'tests' folder, because it (a) makes more sense to put them there, and (b) it prevents any confusion about who's in charge of node_modules. --- tests/wdio/.gitignore | 112 ++++++++++++++++++ .../wdio}/Makefile | 2 - .../wdio}/lib/idiom.js | 0 .../wdio}/lib/utils.js | 0 .../wdio}/package-lock.json | 0 .../wdio}/package.json | 0 .../wdio}/scripts/check_local_chromedriver | 0 .../wdio}/scripts/update_local_chromedriver | 0 .../wdio}/tests/application-plus-ldap.test.js | 0 .../wdio}/wdio.conf-safari.js | 0 .../wdio}/wdio.conf.js | 0 web/authentik-live-tests/.gitignore | 1 - ...k-application-wizard-commit-application.ts | 1 - web/src/admin/applications/wizard/steps.ts | 18 +-- .../ak-wizard-main/ak-wizard-frame.ts | 107 +++++++++-------- .../ak-wizard-main/ak-wizard-main.ts | 111 ++++++++++------- .../ak-wizard-main/commonWizardButtons.ts | 13 ++ .../ak-wizard-main/stories/ak-demo-wizard.ts | 3 +- .../stories/ak-wizard-main.stories.ts | 14 +-- web/src/components/ak-wizard-main/types.ts | 35 +++++- 20 files changed, 296 insertions(+), 121 deletions(-) create mode 100644 tests/wdio/.gitignore rename {web/authentik-live-tests => tests/wdio}/Makefile (90%) rename {web/authentik-live-tests => tests/wdio}/lib/idiom.js (100%) rename {web/authentik-live-tests => tests/wdio}/lib/utils.js (100%) rename {web/authentik-live-tests => tests/wdio}/package-lock.json (100%) rename {web/authentik-live-tests => tests/wdio}/package.json (100%) rename {web/authentik-live-tests => tests/wdio}/scripts/check_local_chromedriver (100%) rename {web/authentik-live-tests => tests/wdio}/scripts/update_local_chromedriver (100%) rename {web/authentik-live-tests => tests/wdio}/tests/application-plus-ldap.test.js (100%) rename {web/authentik-live-tests => tests/wdio}/wdio.conf-safari.js (100%) rename {web/authentik-live-tests => tests/wdio}/wdio.conf.js (100%) delete mode 100644 web/authentik-live-tests/.gitignore create mode 100644 web/src/components/ak-wizard-main/commonWizardButtons.ts diff --git a/tests/wdio/.gitignore b/tests/wdio/.gitignore new file mode 100644 index 000000000..8a156a97e --- /dev/null +++ b/tests/wdio/.gitignore @@ -0,0 +1,112 @@ +reports/ + +# Created by https://www.gitignore.io/api/node +# Edit at https://www.gitignore.io/?templates=node + +### Node ### +# Logs +logs +*.log +npm-debug.log* +yarn-debug.log* +yarn-error.log* +lerna-debug.log* + +# Diagnostic reports (https://nodejs.org/api/report.html) +report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json + +# Runtime data +pids +*.pid +*.seed +*.pid.lock + +# Directory for instrumented libs generated by jscoverage/JSCover +lib-cov + +# Coverage directory used by tools like istanbul +coverage +*.lcov + +# nyc test coverage +.nyc_output + +# Grunt intermediate storage (https://gruntjs.com/creating-plugins#storing-task-files) +.grunt + +# Bower dependency directory (https://bower.io/) +bower_components + +# node-waf configuration +.lock-wscript + +# Compiled binary addons (https://nodejs.org/api/addons.html) +build/Release + +# Dependency directories +node_modules/ +jspm_packages/ + +# TypeScript v1 declaration files +typings/ + +# TypeScript cache +*.tsbuildinfo + +# Optional npm cache directory +.npm + +# Optional eslint cache +.eslintcache + +# Optional REPL history +.node_repl_history + +# Output of 'npm pack' +*.tgz + +# Yarn Integrity file +.yarn-integrity + +# dotenv environment variables file +.env +.env.test + +# parcel-bundler cache (https://parceljs.org/) +.cache + +# next.js build output +.next + +# nuxt.js build output +.nuxt +dist + +# Uncomment the public line if your project uses Gatsby +# https://nextjs.org/blog/next-9-1#public-directory-support +# https://create-react-app.dev/docs/using-the-public-folder/#docsNav +# public + +# Storybook build outputs +.out +.storybook-out + +# vuepress build output +.vuepress/dist + +# Serverless directories +.serverless/ + +# FuseBox cache +.fusebox/ + +# DynamoDB Local files +.dynamodb/ + +# Temporary folders +tmp/ +temp/ + +# End of https://www.gitignore.io/api/node +api/** +storybook-static/ diff --git a/web/authentik-live-tests/Makefile b/tests/wdio/Makefile similarity index 90% rename from web/authentik-live-tests/Makefile rename to tests/wdio/Makefile index 234027565..7024edb27 100644 --- a/web/authentik-live-tests/Makefile +++ b/tests/wdio/Makefile @@ -4,8 +4,6 @@ help: ## Print out this help message. sort -nr | head -1) && \ perl -ne "m/^((\w|-)*):.*##\s*(.*)/ && print(sprintf(\"%s: %s\t%s\n\", \$$1, \" \"x($$M-length(\$$1)), \$$3))" Makefile @echo "" - @echo "Set env NODE_ENV to 'development' to run this against the dev server" - @echo "" .PHONY: update-local-chromedriver update-local-chromedriver: ## Update the chrome driver to match the local chrome version, restoring package.json diff --git a/web/authentik-live-tests/lib/idiom.js b/tests/wdio/lib/idiom.js similarity index 100% rename from web/authentik-live-tests/lib/idiom.js rename to tests/wdio/lib/idiom.js diff --git a/web/authentik-live-tests/lib/utils.js b/tests/wdio/lib/utils.js similarity index 100% rename from web/authentik-live-tests/lib/utils.js rename to tests/wdio/lib/utils.js diff --git a/web/authentik-live-tests/package-lock.json b/tests/wdio/package-lock.json similarity index 100% rename from web/authentik-live-tests/package-lock.json rename to tests/wdio/package-lock.json diff --git a/web/authentik-live-tests/package.json b/tests/wdio/package.json similarity index 100% rename from web/authentik-live-tests/package.json rename to tests/wdio/package.json diff --git a/web/authentik-live-tests/scripts/check_local_chromedriver b/tests/wdio/scripts/check_local_chromedriver similarity index 100% rename from web/authentik-live-tests/scripts/check_local_chromedriver rename to tests/wdio/scripts/check_local_chromedriver diff --git a/web/authentik-live-tests/scripts/update_local_chromedriver b/tests/wdio/scripts/update_local_chromedriver similarity index 100% rename from web/authentik-live-tests/scripts/update_local_chromedriver rename to tests/wdio/scripts/update_local_chromedriver diff --git a/web/authentik-live-tests/tests/application-plus-ldap.test.js b/tests/wdio/tests/application-plus-ldap.test.js similarity index 100% rename from web/authentik-live-tests/tests/application-plus-ldap.test.js rename to tests/wdio/tests/application-plus-ldap.test.js diff --git a/web/authentik-live-tests/wdio.conf-safari.js b/tests/wdio/wdio.conf-safari.js similarity index 100% rename from web/authentik-live-tests/wdio.conf-safari.js rename to tests/wdio/wdio.conf-safari.js diff --git a/web/authentik-live-tests/wdio.conf.js b/tests/wdio/wdio.conf.js similarity index 100% rename from web/authentik-live-tests/wdio.conf.js rename to tests/wdio/wdio.conf.js diff --git a/web/authentik-live-tests/.gitignore b/web/authentik-live-tests/.gitignore deleted file mode 100644 index a9a1bd38a..000000000 --- a/web/authentik-live-tests/.gitignore +++ /dev/null @@ -1 +0,0 @@ -reports/ diff --git a/web/src/admin/applications/wizard/commit/ak-application-wizard-commit-application.ts b/web/src/admin/applications/wizard/commit/ak-application-wizard-commit-application.ts index 64fcf7233..95e7798a1 100644 --- a/web/src/admin/applications/wizard/commit/ak-application-wizard-commit-application.ts +++ b/web/src/admin/applications/wizard/commit/ak-application-wizard-commit-application.ts @@ -103,7 +103,6 @@ export class ApplicationWizardCommitApplication extends BasePanel { Promise.allSettled([network, timeout]).then(([network_resolution]) => { if (network_resolution.status === "rejected") { this.commitState = errorState; - console.log(network_resolution.reason); return; } diff --git a/web/src/admin/applications/wizard/steps.ts b/web/src/admin/applications/wizard/steps.ts index 629402c25..e1f6171fb 100644 --- a/web/src/admin/applications/wizard/steps.ts +++ b/web/src/admin/applications/wizard/steps.ts @@ -1,4 +1,11 @@ import { WizardStep } from "@goauthentik/components/ak-wizard-main"; +import { + BackStep, + CancelWizard, + CloseWizard, + NextStep, + SubmitStep, +} from "@goauthentik/components/ak-wizard-main/commonWizardButtons"; import { msg } from "@lit/localize"; import { html } from "lit"; @@ -15,7 +22,7 @@ export const steps: WizardStep[] = [ renderer: () => html``, disabled: false, - nextButtonLabel: msg("Next"), + buttons: [NextStep, CancelWizard], valid: true, }, { @@ -24,8 +31,7 @@ export const steps: WizardStep[] = [ renderer: () => html``, disabled: false, - nextButtonLabel: msg("Next"), - backButtonLabel: msg("Back"), + buttons: [NextStep, BackStep, CancelWizard], valid: true, }, { @@ -34,8 +40,7 @@ export const steps: WizardStep[] = [ renderer: () => html``, disabled: true, - nextButtonLabel: msg("Next"), - backButtonLabel: msg("Back"), + buttons: [SubmitStep, BackStep, CancelWizard], valid: true, }, { @@ -44,8 +49,7 @@ export const steps: WizardStep[] = [ renderer: () => html``, disabled: true, - nextButtonLabel: msg("Submit"), - backButtonLabel: msg("Back"), + buttons: [BackStep, CancelWizard], valid: true, }, ]; diff --git a/web/src/components/ak-wizard-main/ak-wizard-frame.ts b/web/src/components/ak-wizard-main/ak-wizard-frame.ts index 51d93d3b7..0237a31ff 100644 --- a/web/src/components/ak-wizard-main/ak-wizard-frame.ts +++ b/web/src/components/ak-wizard-main/ak-wizard-frame.ts @@ -5,15 +5,16 @@ import { msg } from "@lit/localize"; import { customElement, property, query } from "@lit/reactive-element/decorators.js"; import { html, nothing } from "lit"; import { classMap } from "lit/directives/class-map.js"; +import { map } from "lit/directives/map.js"; import PFWizard from "@patternfly/patternfly/components/Wizard/wizard.css"; -import type { WizardStep } from "./types"; +import { type WizardButton, type WizardStep } from "./types"; /** * AKWizardFrame is the main container for displaying Wizard pages. * - * AKWizardFrame is one component of a total Wizard development environment. It provides the header, + * AKWizardFrame is one component of a Wizard development environment. It provides the header, * titled navigation sidebar, and bottom row button bar. It takes its cues about what to render from * two data structure, `this.steps: WizardStep[]`, which lists all the current steps *in order* and * doesn't care otherwise about their structure, and `this.currentStep: WizardStep` which must be a @@ -34,9 +35,7 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { return [...super.styles, PFWizard]; } - @property({ type: Boolean }) - canCancel = true; - + /* Prop-drilled. Do not alter. */ @property() header?: string; @@ -49,28 +48,26 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { @property({ attribute: false, type: Array }) steps!: WizardStep[]; - @property({ attribute: false, type: Object }) - currentStep!: WizardStep; + @property({ attribute: false, type: Number }) + currentStep!: number; @query("#main-content *:first-child") content!: HTMLElement; - reset() { - this.open = false; + @property({ type: Boolean }) + canCancel!: boolean; + + get step() { + const step = this.steps[this.currentStep]; + if (!step) { + throw new Error(`Request for step that does not exist: ${this.currentStep}`); + } + return step; } - get maxStep() { - return this.steps.length - 1; - } - - get nextStep() { - const idx = this.steps.findIndex((step) => step === this.currentStep); - return idx < this.maxStep ? this.steps[idx + 1] : undefined; - } - - get backStep() { - const idx = this.steps.findIndex((step) => step === this.currentStep); - return idx > 0 ? this.steps[idx - 1] : undefined; + constructor() { + super(); + this.renderButtons = this.renderButtons.bind(this); } renderModalInner() { @@ -100,7 +97,7 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { class="pf-c-button pf-m-plain pf-c-wizard__close" type="button" aria-label="${msg("Close")}" - @click=${() => this.reset()} + @click=${() => this.dispatchCustomEvent(this.eventName, { command: "close" })} > `; @@ -109,15 +106,15 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { renderNavigation() { return html``; } - renderNavigationStep(step: WizardStep) { + renderNavigationStep(step: WizardStep, idx: number) { const buttonClasses = { "pf-c-wizard__nav-link": true, - "pf-m-current": step.id === this.currentStep.id, + "pf-m-current": idx === this.currentStep, }; return html` @@ -125,7 +122,8 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { @@ -137,48 +135,53 @@ export class AkWizardFrame extends CustomEmitterElement(ModalButton) { // independent context. renderMainSection() { return html`
-
- ${this.currentStep.renderer()} -
+
${this.step.renderer()}
`; } renderFooter() { return html` `; } - renderFooterNextButton(nextStep: WizardStep) { + renderButtons([label, command]: WizardButton) { + switch (command) { + case "next": + return this.renderButton(label, "pf-m-primary", command); + case "back": + return this.renderButton(label, "pf-m-secondary", command); + case "close": + return this.renderLink(label, "pf-m-link"); + default: + throw new Error(`Button type not understood: ${command} for ${label}`); + } + } + + renderButton(label: string, classname: string, command: string) { + const buttonClasses = { "pf-c-button": true, [classname]: true }; return html``; } - renderFooterBackButton(backStep: WizardStep) { - return html` `; - } - - renderFooterCancelButton() { + renderLink(label: string, classname: string) { + const buttonClasses = { "pf-c-button": true, [classname]: true }; return html`
-
`; } diff --git a/web/src/components/ak-wizard-main/ak-wizard-main.ts b/web/src/components/ak-wizard-main/ak-wizard-main.ts index 1189882cc..8443799a2 100644 --- a/web/src/components/ak-wizard-main/ak-wizard-main.ts +++ b/web/src/components/ak-wizard-main/ak-wizard-main.ts @@ -26,11 +26,22 @@ const hasValidator = (v: any): v is Required> => * * @element ak-wizard-main * - * This is the entry point for the wizard. Its tasks are: + * This is the controller for a multi-form wizard. It provides an interface for describing a pop-up + * (modal) wizard, the contents of which are independent of the navigation. This controller only + * handles the navigation. + * + * Each step (see the `types.ts` file) provides label, a "currently valid" boolean, a "disabled" + * boolean, a function that returns the HTML of the object to be rendered, a `disabled` flag + * indicating + + Its tasks are: * - keep the collection of steps * - maintain the open/close status of the modal * - listens for navigation events * - if a navigation event is valid, switch to the panel requested + * + * + */ @customElement("ak-wizard-main") @@ -56,7 +67,7 @@ export class AkWizardMain extends CustomListenerElement(AKElement) { * @attribute */ @state() - currentStep!: WizardStep; + currentStep: number = 0; constructor() { super(); @@ -71,14 +82,6 @@ export class AkWizardMain extends CustomListenerElement(AKElement) { @property({ type: String }) prompt = "Show Wizard"; - /** - * Mostly a control on the ModalButton that summons the wizard component. - * - * @attribute - */ - @property({ type: Boolean, reflect: true }) - open = false; - /** * The text of the header on the wizard, upper bar. * @@ -95,18 +98,17 @@ export class AkWizardMain extends CustomListenerElement(AKElement) { @property() description?: string; + /** + * Whether or not to show the "cancel" button in the wizard. + * + * @attribute + */ + @property({ type: Boolean }) + canCancel!: boolean; + @query("ak-wizard-frame") frame!: AkWizardFrame; - // Guarantee that if the current step was not passed in by the client, that we know - // and set to the first step. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - willUpdate(_changedProperties: Map) { - if (this.currentStep === undefined) { - this.currentStep = this.steps[0]; - } - } - connectedCallback() { super.connectedCallback(); this.addCustomListener(this.eventName, this.handleNavigation); @@ -117,30 +119,55 @@ export class AkWizardMain extends CustomListenerElement(AKElement) { super.disconnectedCallback(); } - // Note that we always scan for the valid next step and throw an error if we can't find it. - // There should never be a question that the currentStep is a *valid* step. - // - // TODO: Put a phase in there so that the current step can validate the contents asynchronously - // before setting the currentStep. Especially since setting the currentStep triggers a second - // asynchronous event-- scheduling a re-render of everything interested in the currentStep - // object. - handleNavigation(event: CustomEvent<{ step: string; action: string }>) { - const requestedStep = event.detail.step; - if (!requestedStep) { - throw new Error("Request for next step when no next step is available"); - } - const step = this.steps.find(({ id }) => id === requestedStep); - if (!step) { - throw new Error("Request for next step when no next step is available."); - } - if (event.detail.action === "next" && !this.validated()) { - return false; - } - this.currentStep = step; - return true; + get maxStep() { + return this.steps.length - 1; } - validated() { + get nextStep() { + return this.currentStep < this.maxStep ? this.currentStep + 1 : undefined; + } + + get backStep() { + return this.currentStep > 0 ? this.currentStep - 1 : undefined; + } + + handleNavigation(event: CustomEvent<{ command: string; step?: number }>) { + const command = event.detail.command; + console.log(command); + switch (command) { + case "back": { + if (this.backStep !== undefined && this.steps[this.backStep]) { + this.currentStep = this.backStep; + } + return; + } + case "goto": { + if ( + typeof event.detail.step === "number" && + event.detail.step >= 0 && + event.detail.step <= this.maxStep + ) + this.currentStep = event.detail.step; + return; + } + case "next": { + if ( + this.nextStep && + this.steps[this.nextStep] && + !this.steps[this.nextStep].disabled && + this.validated + ) { + this.currentStep = this.nextStep; + } + return; + } + case "close": { + this.frame.open = this.open; + } + } + } + + get validated() { if (hasValidator(this.frame.content)) { return this.frame.content.validator(); } @@ -150,7 +177,7 @@ export class AkWizardMain extends CustomListenerElement(AKElement) { render() { return html` diff --git a/web/src/components/ak-wizard-main/stories/ak-wizard-main.stories.ts b/web/src/components/ak-wizard-main/stories/ak-wizard-main.stories.ts index e35f9de5e..73c1ecfd9 100644 --- a/web/src/components/ak-wizard-main/stories/ak-wizard-main.stories.ts +++ b/web/src/components/ak-wizard-main/stories/ak-wizard-main.stories.ts @@ -1,6 +1,6 @@ import "@goauthentik/elements/messages/MessageContainer"; import { Meta } from "@storybook/web-components"; - +import { NextStep, BackStep, CancelWizard, CloseWizard } from "../commonWizardButtons"; import { TemplateResult, html } from "lit"; import "../ak-wizard-main"; @@ -37,27 +37,21 @@ const container = (testItem: TemplateResult) => const dummySteps: WizardStep[] = [ { - id: "0", label: "Test Step1", renderer: () => html`

This space intentionally left blank today

`, disabled: false, - valid: true, - nextButtonLabel: "Next", - backButtonLabel: undefined, + buttons: [NextStep, CancelWizard], }, { - id: "1", label: "Test Step 2", renderer: () => html`

This space also intentionally left blank

`, disabled: false, - valid: true, - nextButtonLabel: undefined, - backButtonLabel: "Back", + buttons: [BackStep, CloseWizard], }, ]; export const OnePageWizard = () => { return container( - html` `, + html` `, ); }; diff --git a/web/src/components/ak-wizard-main/types.ts b/web/src/components/ak-wizard-main/types.ts index 2c629f1b2..41830be08 100644 --- a/web/src/components/ak-wizard-main/types.ts +++ b/web/src/components/ak-wizard-main/types.ts @@ -1,13 +1,38 @@ import { TemplateResult } from "lit"; +export type WizardNavCommand = "next" | "back" | "close" | ["goto", number]; + + +// The label of the button, the command the button should execute, and if the button +// should be marked "disabled." +export type WizardButton = [string, WizardNavCommand, boolean?]; + + export interface WizardStep { - id: string; - label: string; - valid: boolean; + // The name of the step, as shown in the navigation. + label: string; + + // A function which returns the html for rendering the actual content of the step, its form and + // such. renderer: () => TemplateResult; + + // A collection of buttons, in render order, that are to be shown in the button bar. The + // semantics of the buttons are simple: 'next' will navigate to currentStep + 1, 'back' will + // navigate to currentStep - 1, 'close' will close the window, and ['goto', number] will + // navigate to a specific step in order. + // + // It is possible for the controlling component that wraps ak-wizard-main to supply a modified + // collection of steps at any time, thus altering the behavior of future steps, or providing a + // tree-like structure to the wizard. + // + // Note that if you change the steps radically (inserting some in front of the currentStep, + // which is something you should never, ever do... never, ever make the customer go backward to + // solve a problem that was your responsibility. "Going back" to fix their own mistakes is, of + // course, their responsibility) you may have to set the currentStep as well. + buttons: WizardButton[]; + + // If this step is "disabled," the prior step's next button will be disabled. disabled: boolean; - nextButtonLabel?: string; - backButtonLabel?: string; } export interface WizardPanel extends HTMLElement {