From 837a880a493b30502dcdb152be6b76eb45575606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Fri, 26 Apr 2024 16:29:32 +0200 Subject: [PATCH 01/13] Storage: show skeletons only when needed Also display progress for loading the installation device --- .../storage/DeviceSelectionDialog.jsx | 130 ++++++++++-------- .../storage/InstallationDeviceField.jsx | 1 + web/src/components/storage/ProposalPage.jsx | 39 +++++- .../storage/ProposalResultSection.jsx | 16 ++- .../storage/ProposalSettingsSection.jsx | 61 +++++--- 5 files changed, 162 insertions(+), 85 deletions(-) diff --git a/web/src/components/storage/DeviceSelectionDialog.jsx b/web/src/components/storage/DeviceSelectionDialog.jsx index 5efdeeb019..245b90515b 100644 --- a/web/src/components/storage/DeviceSelectionDialog.jsx +++ b/web/src/components/storage/DeviceSelectionDialog.jsx @@ -26,7 +26,7 @@ import { Form } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { deviceChildren } from "~/components/storage/utils"; -import { ControlledPanels as Panels, Popup } from "~/components/core"; +import { ControlledPanels as Panels, If, Popup, SectionSkeleton } from "~/components/core"; import { DeviceSelectorTable } from "~/components/storage"; import { noop } from "~/utils"; @@ -52,6 +52,7 @@ const OPTIONS_NAME = "selection-mode"; * @param {StorageDevice[]} props.targetPVDevices * @param {StorageDevice[]} props.devices - The actions to perform in the system. * @param {boolean} [props.isOpen=false] - Whether the dialog is visible or not. + * @param {boolean} [props.isLoading=false] - Whether loading the data is in progress * @param {() => void} [props.onCancel=noop] * @param {(target: Target) => void} [props.onAccept=noop] * @@ -67,6 +68,7 @@ export default function DeviceSelectionDialog({ targetPVDevices: defaultPVDevices, devices, isOpen, + isLoading, onCancel = noop, onAccept = noop, ...props @@ -95,6 +97,11 @@ export default function DeviceSelectionDialog({ return true; }; + // change the initial `undefined` state when receiving the real data + if (!target && defaultTarget) { setTarget(defaultTarget) } + if (!targetDevice && defaultTargetDevice) { setTargetDevice(defaultTargetDevice) } + if (!targetPVDevices && defaultPVDevices) { setTargetPVDevices(defaultPVDevices) } + const isDeviceSelectable = (device) => device.isDrive || device.type === "md"; // TRANSLATORS: description for using plain partitions for installing the @@ -118,63 +125,70 @@ devices.").split(/[[\]]/); inlineSize="large" {...props} > -
- - - - {_("Select a disk")} - - - {_("Create an LVM Volume Group")} - - - - - {msgStart1} - {msgBold1} - {msgEnd1} - - - - - - {msgStart2} - {msgBold2} - {msgEnd2} - - - - -
+ } + else={ +
+ + + + {_("Select a disk")} + + + {_("Create an LVM Volume Group")} + + + + + {msgStart1} + {msgBold1} + {msgEnd1} + + + + + + {msgStart2} + {msgBold2} + {msgEnd2} + + + + +
+ } + /> + diff --git a/web/src/components/storage/InstallationDeviceField.jsx b/web/src/components/storage/InstallationDeviceField.jsx index eb4fb22988..35e67e504e 100644 --- a/web/src/components/storage/InstallationDeviceField.jsx +++ b/web/src/components/storage/InstallationDeviceField.jsx @@ -127,6 +127,7 @@ export default function InstallationDeviceField({ then={ { } case "STOP_LOADING" : { - return { ...state, loading: false }; + // reset the changing value after the refresh is finished + return { ...state, loading: false, changing: undefined }; } case "UPDATE_AVAILABLE_DEVICES": { @@ -76,8 +79,8 @@ const reducer = (state, action) => { } case "UPDATE_SETTINGS": { - const { settings } = action.payload; - return { ...state, settings }; + const { settings, changing } = action.payload; + return { ...state, settings, changing }; } case "UPDATE_DEVICES": { @@ -96,6 +99,30 @@ const reducer = (state, action) => { } }; +/** + * Which UI item is being changed by user + */ +export const CHANGING = Object.freeze({ + ENCRYPTION: Symbol("encryption"), + TARGET: Symbol("target"), + VOLUMES: Symbol("volumes"), + POLICY: Symbol("policy"), + BOOT: Symbol("boot"), +}); + +// mapping of not affected values for settings components +// key: component name +// value: list of items which can be changed without affecting +// the state of the component +export const NOT_AFFECTED = { + // the EncryptionField shows the skeleton only during initial load, + // it does not depend on any changed item and does not show skeleton later + InstallationDeviceField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.POLICY, CHANGING.VOLUMES], + PartitionsField: [CHANGING.ENCRYPTION], + SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.VOLUMES, CHANGING.TARGET], + ProposalResultSection: [CHANGING.ENCRYPTION], +}; + export default function ProposalPage() { const { storage: client } = useInstallerClient(); const { cancellablePromise } = useCancellablePromise(); @@ -208,10 +235,10 @@ export default function ProposalPage() { } }, [client, load, state.settings]); - const changeSettings = async (settings) => { + const changeSettings = async (changing, settings) => { const newSettings = { ...state.settings, ...settings }; - dispatch({ type: "UPDATE_SETTINGS", payload: { settings: newSettings } }); + dispatch({ type: "UPDATE_SETTINGS", payload: { settings: newSettings, changing } }); calculate(newSettings).catch(console.error); }; @@ -236,6 +263,7 @@ export default function ProposalPage() { settings={state.settings} onChange={changeSettings} isLoading={state.loading} + changing={state.changing} /> ); diff --git a/web/src/components/storage/ProposalResultSection.jsx b/web/src/components/storage/ProposalResultSection.jsx index 741a5c6c4a..ed3197e581 100644 --- a/web/src/components/storage/ProposalResultSection.jsx +++ b/web/src/components/storage/ProposalResultSection.jsx @@ -26,6 +26,7 @@ import { Button, Skeleton } from "@patternfly/react-core"; import { sprintf } from "sprintf-js"; import { _, n_ } from "~/i18n"; import { deviceChildren, deviceSize } from "~/components/storage/utils"; +import { NOT_AFFECTED } from "~/components/storage/ProposalPage"; import DevicesManager from "~/components/storage/DevicesManager"; import { If, Section, Reminder, Tag, TreeTable } from "~/components/core"; import { ProposalActionsDialog, FilesystemLabel } from "~/components/storage"; @@ -36,6 +37,13 @@ import { ProposalActionsDialog, FilesystemLabel } from "~/components/storage"; * @typedef {import("~/client/mixins").ValidationError} ValidationError */ +/** + * A helper function to decide whether to show the progress skeletons or not + * @param {boolean} loading + * @param {symbol} changing the item which is being changed + */ +const ShowSkeleton = (loading, changing) => loading && !NOT_AFFECTED.ProposalResultSection.includes(changing); + /** * Renders information about planned actions, allowing to check all of them and warning with a * summary about the deletion ones, if any. @@ -251,13 +259,15 @@ const SectionContent = ({ system, staging, actions, errors }) => { * @param {Action[]} [props.actions=[]] * @param {ValidationError[]} [props.errors=[]] - Validation errors * @param {boolean} [props.isLoading=false] - Whether the section content should be rendered as loading + * @param {symbol} [props.changing=undefined] - Which part of the configuration is being changed by user */ export default function ProposalResultSection({ system = [], staging = [], actions = [], errors = [], - isLoading = false + isLoading = false, + changing = undefined }) { if (isLoading) errors = []; const totalActions = actions.length; @@ -274,12 +284,12 @@ export default function ProposalResultSection({
} else={ { + return loading && !NOT_AFFECTED[component].includes(changing); +}; + /** * Section for editing the proposal settings * @component @@ -50,7 +62,8 @@ import SpacePolicyField from "~/components/storage/SpacePolicyField"; * @property {String[]} encryptionMethods * @property {Volume[]} volumeTemplates * @property {boolean} [isLoading=false] - * @property {(settings: object) => void} onChange + * @property {symbol} [changing=undefined] Which part of the configuration is being changed by user + * @property {(changing: symbol, settings: object) => void} onChange * * @param {ProposalSettingsSectionProps} props */ @@ -60,41 +73,51 @@ export default function ProposalSettingsSection({ encryptionMethods, volumeTemplates, isLoading = false, + changing = undefined, onChange }) { /** @param {import("~/components/storage/InstallationDeviceField").TargetConfig} targetConfig */ const changeTarget = ({ target, targetDevice, targetPVDevices }) => { - onChange({ - target, - targetDevice: targetDevice?.name, - targetPVDevices: targetPVDevices.map(d => d.name) - }); + onChange( + CHANGING.TARGET, + { + target, + targetDevice: targetDevice?.name, + targetPVDevices: targetPVDevices.map(d => d.name) + } + ); }; /** @param {import("~/components/storage/EncryptionField").EncryptionConfig} encryptionConfig */ const changeEncryption = ({ password, method }) => { - onChange({ encryptionPassword: password, encryptionMethod: method }); + onChange(CHANGING.ENCRYPTION, { encryptionPassword: password, encryptionMethod: method }); }; /** @param {Volume[]} volumes */ const changeVolumes = (volumes) => { - onChange({ volumes }); + onChange(CHANGING.VOLUMES, { volumes }); }; /** @param {import("~/components/storage/SpacePolicyField").SpacePolicyConfig} spacePolicyConfig */ const changeSpacePolicy = ({ spacePolicy, spaceActions }) => { - onChange({ - spacePolicy: spacePolicy.id, - spaceActions - }); + onChange( + CHANGING.POLICY, + { + spacePolicy: spacePolicy.id, + spaceActions + } + ); }; /** @param {import("~/components/storage/PartitionsField").BootConfig} bootConfig */ const changeBoot = ({ configureBoot, bootDevice }) => { - onChange({ - configureBoot, - bootDevice: bootDevice?.name - }); + onChange( + CHANGING.BOOT, + { + configureBoot, + bootDevice: bootDevice?.name + } + ); }; /** @@ -120,7 +143,7 @@ export default function ProposalSettingsSection({ targetDevice={targetDevice} targetPVDevices={targetPVDevices} devices={availableDevices} - isLoading={isLoading} + isLoading={ShowSkeleton(isLoading, "InstallationDeviceField", changing)} onChange={changeTarget} /> @@ -147,7 +170,7 @@ export default function ProposalSettingsSection({ policy={spacePolicy} actions={spaceActions} devices={installationDevices} - isLoading={isLoading} + isLoading={ShowSkeleton(isLoading, "SpacePolicyField", changing)} onChange={changeSpacePolicy} />
From d093d361cde86b008f305c193032e3236a3ec799 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Fri, 26 Apr 2024 16:58:05 +0200 Subject: [PATCH 02/13] More comments --- web/src/components/storage/ProposalResultSection.jsx | 2 +- web/src/components/storage/ProposalSettingsSection.jsx | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/web/src/components/storage/ProposalResultSection.jsx b/web/src/components/storage/ProposalResultSection.jsx index ed3197e581..d368f876cd 100644 --- a/web/src/components/storage/ProposalResultSection.jsx +++ b/web/src/components/storage/ProposalResultSection.jsx @@ -39,7 +39,7 @@ import { ProposalActionsDialog, FilesystemLabel } from "~/components/storage"; /** * A helper function to decide whether to show the progress skeletons or not - * @param {boolean} loading + * @param {boolean} loading loading status * @param {symbol} changing the item which is being changed */ const ShowSkeleton = (loading, changing) => loading && !NOT_AFFECTED.ProposalResultSection.includes(changing); diff --git a/web/src/components/storage/ProposalSettingsSection.jsx b/web/src/components/storage/ProposalSettingsSection.jsx index 5e754a1a01..8c5d9f0313 100644 --- a/web/src/components/storage/ProposalSettingsSection.jsx +++ b/web/src/components/storage/ProposalSettingsSection.jsx @@ -43,8 +43,9 @@ import SpacePolicyField from "~/components/storage/SpacePolicyField"; /** * A helper function to decide whether to show the progress skeletons or not - * @param {boolean} loading - * @param {string} component + * for the specified component + * @param {boolean} loading loading status + * @param {string} component name of the component * @param {symbol} changing the item which is being changed * @returns {boolean} true if the skeleton should be displayed, false otherwise */ @@ -62,7 +63,7 @@ const ShowSkeleton = (loading, component, changing) => { * @property {String[]} encryptionMethods * @property {Volume[]} volumeTemplates * @property {boolean} [isLoading=false] - * @property {symbol} [changing=undefined] Which part of the configuration is being changed by user + * @property {symbol} [changing=undefined] which part of the configuration is being changed by user * @property {(changing: symbol, settings: object) => void} onChange * * @param {ProposalSettingsSectionProps} props From f535c4dd4ee99b2aecb1ef30536878887c09b161 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Mon, 29 Apr 2024 09:11:01 +0200 Subject: [PATCH 03/13] Simplify screen reader texts --- web/src/components/storage/InstallationDeviceField.jsx | 2 +- web/src/components/storage/ProposalResultSection.jsx | 2 +- web/src/components/storage/SpacePolicyField.jsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/web/src/components/storage/InstallationDeviceField.jsx b/web/src/components/storage/InstallationDeviceField.jsx index 35e67e504e..bf326dc752 100644 --- a/web/src/components/storage/InstallationDeviceField.jsx +++ b/web/src/components/storage/InstallationDeviceField.jsx @@ -109,7 +109,7 @@ export default function InstallationDeviceField({ let value; if (isLoading || !target) - value = ; + value = ; else value = targetValue(target, targetDevice, targetPVDevices); diff --git a/web/src/components/storage/ProposalResultSection.jsx b/web/src/components/storage/ProposalResultSection.jsx index d368f876cd..8bd711a755 100644 --- a/web/src/components/storage/ProposalResultSection.jsx +++ b/web/src/components/storage/ProposalResultSection.jsx @@ -215,7 +215,7 @@ const DevicesTreeTable = ({ devicesManager }) => { const ResultSkeleton = () => { return ( <> - + diff --git a/web/src/components/storage/SpacePolicyField.jsx b/web/src/components/storage/SpacePolicyField.jsx index 7858df0fc7..991c2f8d39 100644 --- a/web/src/components/storage/SpacePolicyField.jsx +++ b/web/src/components/storage/SpacePolicyField.jsx @@ -72,7 +72,7 @@ export default function SpacePolicyField({ let value; if (isLoading || !policy) { - value = ; + value = ; } else if (policy.summaryLabels.length === 1) { // eslint-disable-next-line agama-i18n/string-literals value = _(policy.summaryLabels[0]); From 358e04a59f601c1b1dd5822f0ea5fc82effbe9b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Mon, 29 Apr 2024 13:30:44 +0200 Subject: [PATCH 04/13] Always refresh the proposal result --- web/src/components/storage/ProposalPage.jsx | 5 ++--- .../components/storage/ProposalResultSection.jsx | 15 +++------------ 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index 85328b4792..e736563aa1 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -116,11 +116,11 @@ export const CHANGING = Object.freeze({ // the state of the component export const NOT_AFFECTED = { // the EncryptionField shows the skeleton only during initial load, - // it does not depend on any changed item and does not show skeleton later + // it does not depend on any changed item and does not show skeleton later. + // the ProposalResultSection is refreshed always InstallationDeviceField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.POLICY, CHANGING.VOLUMES], PartitionsField: [CHANGING.ENCRYPTION], SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.VOLUMES, CHANGING.TARGET], - ProposalResultSection: [CHANGING.ENCRYPTION], }; export default function ProposalPage() { @@ -271,7 +271,6 @@ export default function ProposalPage() { actions={state.actions} errors={state.errors} isLoading={state.loading} - changing={state.changing} /> ); diff --git a/web/src/components/storage/ProposalResultSection.jsx b/web/src/components/storage/ProposalResultSection.jsx index 8bd711a755..913d2cdbc7 100644 --- a/web/src/components/storage/ProposalResultSection.jsx +++ b/web/src/components/storage/ProposalResultSection.jsx @@ -26,7 +26,6 @@ import { Button, Skeleton } from "@patternfly/react-core"; import { sprintf } from "sprintf-js"; import { _, n_ } from "~/i18n"; import { deviceChildren, deviceSize } from "~/components/storage/utils"; -import { NOT_AFFECTED } from "~/components/storage/ProposalPage"; import DevicesManager from "~/components/storage/DevicesManager"; import { If, Section, Reminder, Tag, TreeTable } from "~/components/core"; import { ProposalActionsDialog, FilesystemLabel } from "~/components/storage"; @@ -37,13 +36,6 @@ import { ProposalActionsDialog, FilesystemLabel } from "~/components/storage"; * @typedef {import("~/client/mixins").ValidationError} ValidationError */ -/** - * A helper function to decide whether to show the progress skeletons or not - * @param {boolean} loading loading status - * @param {symbol} changing the item which is being changed - */ -const ShowSkeleton = (loading, changing) => loading && !NOT_AFFECTED.ProposalResultSection.includes(changing); - /** * Renders information about planned actions, allowing to check all of them and warning with a * summary about the deletion ones, if any. @@ -266,8 +258,7 @@ export default function ProposalResultSection({ staging = [], actions = [], errors = [], - isLoading = false, - changing = undefined + isLoading = false }) { if (isLoading) errors = []; const totalActions = actions.length; @@ -284,12 +275,12 @@ export default function ProposalResultSection({
} else={ Date: Mon, 29 Apr 2024 13:50:30 +0200 Subject: [PATCH 05/13] Update test --- web/src/components/storage/InstallationDeviceField.test.jsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/web/src/components/storage/InstallationDeviceField.test.jsx b/web/src/components/storage/InstallationDeviceField.test.jsx index 25b8d8c717..2b3d1fc234 100644 --- a/web/src/components/storage/InstallationDeviceField.test.jsx +++ b/web/src/components/storage/InstallationDeviceField.test.jsx @@ -97,8 +97,10 @@ describe("when set as loading", () => { }); it("renders a loading hint", () => { - installerRender(); - screen.getByText("Waiting for information about selected device"); + const { container } = installerRender(); + + // a PF skeleton is displayed + expect(container.querySelector("div.pf-v5-c-skeleton")).toBeVisible(); }); }); From 4583eafccbb7abebe34aa5c613c7ea103f26e2f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Mon, 29 Apr 2024 18:34:19 +0200 Subject: [PATCH 06/13] Code review fixes --- web/src/components/storage/BootConfigField.jsx | 4 ++-- web/src/components/storage/ProposalSettingsSection.jsx | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/web/src/components/storage/BootConfigField.jsx b/web/src/components/storage/BootConfigField.jsx index b62ccf14fb..34f5b23422 100644 --- a/web/src/components/storage/BootConfigField.jsx +++ b/web/src/components/storage/BootConfigField.jsx @@ -87,8 +87,8 @@ export default function BootConfigField({ onChange({ configureBoot, bootDevice }); }; - if (isLoading) { - return ; + if (isLoading && configureBoot === undefined) { + return ; } let value; diff --git a/web/src/components/storage/ProposalSettingsSection.jsx b/web/src/components/storage/ProposalSettingsSection.jsx index 8c5d9f0313..6e79c68633 100644 --- a/web/src/components/storage/ProposalSettingsSection.jsx +++ b/web/src/components/storage/ProposalSettingsSection.jsx @@ -49,7 +49,7 @@ import SpacePolicyField from "~/components/storage/SpacePolicyField"; * @param {symbol} changing the item which is being changed * @returns {boolean} true if the skeleton should be displayed, false otherwise */ -const ShowSkeleton = (loading, component, changing) => { +const showSkeleton = (loading, component, changing) => { return loading && !NOT_AFFECTED[component].includes(changing); }; @@ -144,7 +144,7 @@ export default function ProposalSettingsSection({ targetDevice={targetDevice} targetPVDevices={targetPVDevices} devices={availableDevices} - isLoading={ShowSkeleton(isLoading, "InstallationDeviceField", changing)} + isLoading={showSkeleton(isLoading, "InstallationDeviceField", changing)} onChange={changeTarget} /> @@ -171,7 +171,7 @@ export default function ProposalSettingsSection({ policy={spacePolicy} actions={spaceActions} devices={installationDevices} - isLoading={ShowSkeleton(isLoading, "SpacePolicyField", changing)} + isLoading={showSkeleton(isLoading, "SpacePolicyField", changing)} onChange={changeSpacePolicy} />
From 83eccdf7b88abb52e5ba52a450f502fd75c9c428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Tue, 30 Apr 2024 10:07:46 +0200 Subject: [PATCH 07/13] Review fixes --- web/src/components/core/If.jsx | 8 +-- web/src/components/core/If.test.jsx | 10 ++++ .../storage/DeviceSelectionDialog.jsx | 7 ++- web/src/components/storage/ProposalPage.jsx | 4 +- .../components/storage/SpacePolicyDialog.jsx | 51 ++++++++++++------- .../components/storage/SpacePolicyField.jsx | 1 + 6 files changed, 56 insertions(+), 25 deletions(-) diff --git a/web/src/components/core/If.jsx b/web/src/components/core/If.jsx index c53991d1f8..0e384cbc62 100644 --- a/web/src/components/core/If.jsx +++ b/web/src/components/core/If.jsx @@ -60,13 +60,15 @@ * * @param {object} props * @param {boolean} props.condition - * @param {JSX.Element} [props.then=null] - the content to be rendered when the condition is true - * @param {JSX.Element} [props.else=null] - the content to be rendered when the condition is false + * @param {(JSX.Element|function():JSX.Element)} [props.then=null] - the content to be rendered when the condition is true + * @param {(JSX.Element|function():JSX.Element)} [props.else=null] - the content to be rendered when the condition is false */ export default function If ({ condition, then: positive = null, else: negative = null }) { - return condition ? positive : negative; + const ret = condition ? positive : negative; + // if the parameter is a function then evaluate it + return (typeof ret === "function") ? ret() : ret; } diff --git a/web/src/components/core/If.test.jsx b/web/src/components/core/If.test.jsx index f9d5c952bb..001e31e2a0 100644 --- a/web/src/components/core/If.test.jsx +++ b/web/src/components/core/If.test.jsx @@ -30,6 +30,11 @@ describe("If", () => { it("renders content given in 'then' prop", () => { plainRender(); + screen.getByText("Hello World!"); + }); + it("renders result of function given in 'then' prop", () => { + plainRender( "Hello World!"} else={() => "Goodbye World!"} />); + screen.getByText("Hello World!"); }); }); @@ -48,6 +53,11 @@ describe("If", () => { it("renders content given in 'else' prop", () => { plainRender(); + screen.getByText("Goodbye World!"); + }); + it("renders result of function given in 'else' prop", () => { + plainRender( "Hello World!"} else={() => "Goodbye World!"} />); + screen.getByText("Goodbye World!"); }); }); diff --git a/web/src/components/storage/DeviceSelectionDialog.jsx b/web/src/components/storage/DeviceSelectionDialog.jsx index 245b90515b..1cfc527063 100644 --- a/web/src/components/storage/DeviceSelectionDialog.jsx +++ b/web/src/components/storage/DeviceSelectionDialog.jsx @@ -26,9 +26,10 @@ import { Form } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { deviceChildren } from "~/components/storage/utils"; -import { ControlledPanels as Panels, If, Popup, SectionSkeleton } from "~/components/core"; +import { ControlledPanels as Panels, If, Popup } from "~/components/core"; import { DeviceSelectorTable } from "~/components/storage"; import { noop } from "~/utils"; +import { Loading } from "~/components/layout"; /** * @typedef {import ("~/client/storage").ProposalTarget} ProposalTarget @@ -127,7 +128,9 @@ devices.").split(/[[\]]/); > } + then={ + + } else={
diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index e736563aa1..16262c5fa6 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -119,8 +119,8 @@ export const NOT_AFFECTED = { // it does not depend on any changed item and does not show skeleton later. // the ProposalResultSection is refreshed always InstallationDeviceField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.POLICY, CHANGING.VOLUMES], - PartitionsField: [CHANGING.ENCRYPTION], - SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.VOLUMES, CHANGING.TARGET], + PartitionsField: [CHANGING.ENCRYPTION, CHANGING.POLICY], + SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.TARGET], }; export default function ProposalPage() { diff --git a/web/src/components/storage/SpacePolicyDialog.jsx b/web/src/components/storage/SpacePolicyDialog.jsx index e0f5162d04..b5e1ca818d 100644 --- a/web/src/components/storage/SpacePolicyDialog.jsx +++ b/web/src/components/storage/SpacePolicyDialog.jsx @@ -27,6 +27,7 @@ import { Form } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { SPACE_POLICIES } from '~/components/storage/utils'; import { If, OptionsPicker, Popup } from "~/components/core"; +import { Loading } from "~/components/layout"; import { noop } from "~/utils"; import { SpaceActionsTable } from '~/components/storage'; @@ -73,6 +74,7 @@ const SpacePolicyPicker = ({ currentPolicy, onChange = noop }) => { * @param {SpaceAction[]} props.actions * @param {StorageDevice[]} props.devices * @param {boolean} [props.isOpen=false] + * @param {boolean} [props.isLoading] * @param {() => void} [props.onCancel=noop] * @param {(spaceConfig: SpaceConfig) => void} [props.onAccept=noop] * @@ -85,6 +87,7 @@ export default function SpacePolicyDialog({ actions: defaultActions, devices, isOpen, + isLoading, onCancel = noop, onAccept = noop, ...props @@ -94,8 +97,11 @@ export default function SpacePolicyDialog({ const [customUsed, setCustomUsed] = useState(false); const [expandedDevices, setExpandedDevices] = useState([]); + if (!policy && defaultPolicy) { setPolicy(defaultPolicy) } + if (!actions && defaultActions) { setActions(defaultActions) } + useEffect(() => { - if (policy.id === "custom") setExpandedDevices(devices); + if (policy && policy.id === "custom") setExpandedDevices(devices); }, [devices, policy, setExpandedDevices]); // The selectors for the space action have to be initialized always to the same value @@ -104,12 +110,12 @@ export default function SpacePolicyDialog({ // Stores whether the custom policy has been used. useEffect(() => { - if (policy.id === "custom" && !customUsed) setCustomUsed(true); + if (policy && policy.id === "custom" && !customUsed) setCustomUsed(true); }, [policy, customUsed, setCustomUsed]); // Resets actions (i.e., sets everything to "keep") if the custom policy has not been used yet. useEffect(() => { - if (policy.id !== "custom" && !customUsed) setActions([]); + if (policy && policy.id !== "custom" && !customUsed) setActions([]); }, [policy, customUsed, setActions]); // Generates the action value according to the policy. @@ -146,21 +152,30 @@ in the devices listed below. Choose how to do it."); inlineSize="large" {...props} > - - - 0} - then={ - - } - /> - + + } + else={ + () => +
+ + 0} + then={ + + } + /> + + } + /> diff --git a/web/src/components/storage/SpacePolicyField.jsx b/web/src/components/storage/SpacePolicyField.jsx index 991c2f8d39..db1cb0d4ff 100644 --- a/web/src/components/storage/SpacePolicyField.jsx +++ b/web/src/components/storage/SpacePolicyField.jsx @@ -95,6 +95,7 @@ in the installation device(s).")} then={ Date: Tue, 30 Apr 2024 14:36:08 +0200 Subject: [PATCH 08/13] Revert the change --- web/src/components/core/If.jsx | 8 ++--- web/src/components/core/If.test.jsx | 10 ------ .../components/storage/SpacePolicyDialog.jsx | 31 +++++++++---------- 3 files changed, 18 insertions(+), 31 deletions(-) diff --git a/web/src/components/core/If.jsx b/web/src/components/core/If.jsx index 0e384cbc62..c53991d1f8 100644 --- a/web/src/components/core/If.jsx +++ b/web/src/components/core/If.jsx @@ -60,15 +60,13 @@ * * @param {object} props * @param {boolean} props.condition - * @param {(JSX.Element|function():JSX.Element)} [props.then=null] - the content to be rendered when the condition is true - * @param {(JSX.Element|function():JSX.Element)} [props.else=null] - the content to be rendered when the condition is false + * @param {JSX.Element} [props.then=null] - the content to be rendered when the condition is true + * @param {JSX.Element} [props.else=null] - the content to be rendered when the condition is false */ export default function If ({ condition, then: positive = null, else: negative = null }) { - const ret = condition ? positive : negative; - // if the parameter is a function then evaluate it - return (typeof ret === "function") ? ret() : ret; + return condition ? positive : negative; } diff --git a/web/src/components/core/If.test.jsx b/web/src/components/core/If.test.jsx index 001e31e2a0..f9d5c952bb 100644 --- a/web/src/components/core/If.test.jsx +++ b/web/src/components/core/If.test.jsx @@ -30,11 +30,6 @@ describe("If", () => { it("renders content given in 'then' prop", () => { plainRender(); - screen.getByText("Hello World!"); - }); - it("renders result of function given in 'then' prop", () => { - plainRender( "Hello World!"} else={() => "Goodbye World!"} />); - screen.getByText("Hello World!"); }); }); @@ -53,11 +48,6 @@ describe("If", () => { it("renders content given in 'else' prop", () => { plainRender(); - screen.getByText("Goodbye World!"); - }); - it("renders result of function given in 'else' prop", () => { - plainRender( "Hello World!"} else={() => "Goodbye World!"} />); - screen.getByText("Goodbye World!"); }); }); diff --git a/web/src/components/storage/SpacePolicyDialog.jsx b/web/src/components/storage/SpacePolicyDialog.jsx index b5e1ca818d..f9b592c4df 100644 --- a/web/src/components/storage/SpacePolicyDialog.jsx +++ b/web/src/components/storage/SpacePolicyDialog.jsx @@ -158,22 +158,21 @@ in the devices listed below. Choose how to do it."); } else={ - () => -
- - 0} - then={ - - } - /> - +
+ + 0} + then={ + + } + /> + } /> From 5d231ca56153112cfc43a9ef230266f3930e7a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Tue, 30 Apr 2024 15:27:34 +0200 Subject: [PATCH 09/13] Add `isLoading` property to the component --- web/src/components/core/Popup.jsx | 5 +- .../storage/DeviceSelectionDialog.jsx | 126 ++++++++---------- .../components/storage/EncryptionField.jsx | 1 + .../storage/EncryptionSettingsDialog.jsx | 11 +- .../components/storage/SpacePolicyDialog.jsx | 38 +++--- 5 files changed, 89 insertions(+), 92 deletions(-) diff --git a/web/src/components/core/Popup.jsx b/web/src/components/core/Popup.jsx index df0b49abe2..ffb09db8dc 100644 --- a/web/src/components/core/Popup.jsx +++ b/web/src/components/core/Popup.jsx @@ -25,6 +25,7 @@ import React from "react"; import { Button, Modal } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { partition } from "~/utils"; +import { Loading } from "~/components/layout"; /** * @typedef {import("@patternfly/react-core").ModalProps} ModalProps @@ -190,12 +191,14 @@ const AncillaryAction = ({ children, ...actionsProps }) => ( * @typedef {object} PopupBaseProps * @property {"auto" | "small" | "medium" | "large"} [blockSize="auto"] - The block/height size for the dialog. Default is "auto". * @property {"auto" | "small" | "medium" | "large"} [inlineSize="medium"] - The inline/width size for the dialog. Default is "medium". + * @property {boolean} [isLoading=false] - Whether the data is loading, if yes it displays a loading indicator instead of the requested content * @typedef {Omit & PopupBaseProps} PopupProps * * @param {PopupProps} props */ const Popup = ({ isOpen = false, + isLoading = false, showClose = false, inlineSize = "medium", blockSize = "auto", @@ -214,7 +217,7 @@ const Popup = ({ actions={actions} className={`${className} block-size-${blockSize} inline-size-${inlineSize}`.trim()} > - {content} + {isLoading ? : content} ); }; diff --git a/web/src/components/storage/DeviceSelectionDialog.jsx b/web/src/components/storage/DeviceSelectionDialog.jsx index 1cfc527063..403f0a88e7 100644 --- a/web/src/components/storage/DeviceSelectionDialog.jsx +++ b/web/src/components/storage/DeviceSelectionDialog.jsx @@ -26,10 +26,9 @@ import { Form } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { deviceChildren } from "~/components/storage/utils"; -import { ControlledPanels as Panels, If, Popup } from "~/components/core"; +import { ControlledPanels as Panels, Popup } from "~/components/core"; import { DeviceSelectorTable } from "~/components/storage"; import { noop } from "~/utils"; -import { Loading } from "~/components/layout"; /** * @typedef {import ("~/client/storage").ProposalTarget} ProposalTarget @@ -122,75 +121,68 @@ devices.").split(/[[\]]/); - - } - else={ -
- - - - {_("Select a disk")} - - - {_("Create an LVM Volume Group")} - - - - - {msgStart1} - {msgBold1} - {msgEnd1} - - - - - - {msgStart2} - {msgBold2} - {msgEnd2} - - - - -
- } - /> +
+ + + + {_("Select a disk")} + + + {_("Create an LVM Volume Group")} + + + + + {msgStart1} + {msgBold1} + {msgEnd1} + + + + + + {msgStart2} + {msgBold2} + {msgEnd2} + + + + +
diff --git a/web/src/components/storage/EncryptionField.jsx b/web/src/components/storage/EncryptionField.jsx index 208b2afcd6..0b2bbc7c40 100644 --- a/web/src/components/storage/EncryptionField.jsx +++ b/web/src/components/storage/EncryptionField.jsx @@ -106,6 +106,7 @@ export default function EncryptionField({ method={method} methods={methods} isOpen={isDialogOpen} + isLoading={isLoading} onCancel={closeDialog} onAccept={onAccept} /> diff --git a/web/src/components/storage/EncryptionSettingsDialog.jsx b/web/src/components/storage/EncryptionSettingsDialog.jsx index fb37b4f598..c580b090cc 100644 --- a/web/src/components/storage/EncryptionSettingsDialog.jsx +++ b/web/src/components/storage/EncryptionSettingsDialog.jsx @@ -53,6 +53,7 @@ directly on its first run."); * @property {string} method - Encryption method. * @property {string[]} methods - Possible encryption methods. * @property {boolean} [isOpen=false] - Whether the dialog is visible or not. + * @property {boolean} [isLoading=false] - Whether the data is loading * @property {() => void} onCancel - Callback to trigger when on cancel action. * @property {(settings: EncryptionSetting) => void} onAccept - Callback to trigger on accept action. * @@ -63,6 +64,7 @@ export default function EncryptionSettingsDialog({ method: methodProp, methods, isOpen = false, + isLoading = false, onCancel, onAccept }) { @@ -73,6 +75,13 @@ export default function EncryptionSettingsDialog({ const [validSettings, setValidSettings] = useState(true); const formId = "encryptionSettingsForm"; + // refresh the state when the real values are loaded + if (method === "" && methodProp !== "") { setMethod(methodProp) } + if (password === "" && passwordProp !== "") { + setPassword(passwordProp); + setIsEnabled(true); + } + useEffect(() => { setValidSettings(!isEnabled || (password.length > 0 && passwordsMatch)); }, [isEnabled, password, passwordsMatch]); @@ -91,7 +100,7 @@ export default function EncryptionSettingsDialog({ }; return ( - + - - } - else={ -
- - 0} - then={ - - } + + + 0} + then={ + - - } - /> + } + /> + From bc2cd452af6acb87698689314139f624910de56f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Tue, 30 Apr 2024 17:04:41 +0200 Subject: [PATCH 10/13] Make the progress message configurable --- web/src/components/core/Popup.jsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/web/src/components/core/Popup.jsx b/web/src/components/core/Popup.jsx index ffb09db8dc..ff547d63dc 100644 --- a/web/src/components/core/Popup.jsx +++ b/web/src/components/core/Popup.jsx @@ -192,6 +192,7 @@ const AncillaryAction = ({ children, ...actionsProps }) => ( * @property {"auto" | "small" | "medium" | "large"} [blockSize="auto"] - The block/height size for the dialog. Default is "auto". * @property {"auto" | "small" | "medium" | "large"} [inlineSize="medium"] - The inline/width size for the dialog. Default is "medium". * @property {boolean} [isLoading=false] - Whether the data is loading, if yes it displays a loading indicator instead of the requested content + * @property {string} [loadingText="Loading data..."] - Text displayed when `isLoading` is set to `true` * @typedef {Omit & PopupBaseProps} PopupProps * * @param {PopupProps} props @@ -199,6 +200,8 @@ const AncillaryAction = ({ children, ...actionsProps }) => ( const Popup = ({ isOpen = false, isLoading = false, + // TRANSLATORS: progress message + loadingText = _("Loading data..."), showClose = false, inlineSize = "medium", blockSize = "auto", @@ -217,7 +220,7 @@ const Popup = ({ actions={actions} className={`${className} block-size-${blockSize} inline-size-${inlineSize}`.trim()} > - {isLoading ? : content} + {isLoading ? : content} ); }; From db04da1e58b849872d8e86d06556fd71cdb19572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 2 May 2024 11:41:49 +0200 Subject: [PATCH 11/13] Reset the values after loading is finished --- .../storage/EncryptionSettingsDialog.jsx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/web/src/components/storage/EncryptionSettingsDialog.jsx b/web/src/components/storage/EncryptionSettingsDialog.jsx index c580b090cc..f5b111456e 100644 --- a/web/src/components/storage/EncryptionSettingsDialog.jsx +++ b/web/src/components/storage/EncryptionSettingsDialog.jsx @@ -73,13 +73,19 @@ export default function EncryptionSettingsDialog({ const [method, setMethod] = useState(methodProp); const [passwordsMatch, setPasswordsMatch] = useState(true); const [validSettings, setValidSettings] = useState(true); + const [wasLoading, setWasLoading] = useState(isLoading); const formId = "encryptionSettingsForm"; - // refresh the state when the real values are loaded - if (method === "" && methodProp !== "") { setMethod(methodProp) } - if (password === "" && passwordProp !== "") { - setPassword(passwordProp); - setIsEnabled(true); + // reset the settings only after loading is finished + if (isLoading && !wasLoading) { setWasLoading(true) } + if (!isLoading && wasLoading) { + setWasLoading(false); + // refresh the state when the real values are loaded + if (method !== methodProp) { setMethod(methodProp) } + if (password !== passwordProp) { + setPassword(passwordProp); + setIsEnabled(passwordProp?.length > 0); + } } useEffect(() => { From da9ec7ddf9d86198c7d4b24590972eec4405031d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 2 May 2024 12:57:21 +0200 Subject: [PATCH 12/13] Update unit test --- web/src/components/core/Popup.test.jsx | 32 +++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/web/src/components/core/Popup.test.jsx b/web/src/components/core/Popup.test.jsx index 2184396eb8..db924b33ad 100644 --- a/web/src/components/core/Popup.test.jsx +++ b/web/src/components/core/Popup.test.jsx @@ -27,8 +27,10 @@ import { installerRender } from "~/test-utils"; import { Popup } from "~/components/core"; let isOpen; +let isLoading; const confirmFn = jest.fn(); const cancelFn = jest.fn(); +const loadingText = "Loading text"; const TestingPopup = (props) => { const [isMounted, setIsMounted] = useState(true); @@ -39,6 +41,8 @@ const TestingPopup = (props) => {

The Popup Content

@@ -55,6 +59,7 @@ describe("Popup", () => { describe("when it is not open", () => { beforeEach(() => { isOpen = false; + isLoading = false; }); it("renders nothing", async () => { @@ -65,9 +70,10 @@ describe("Popup", () => { }); }); - describe("when it is open", () => { + describe("when it is open and not loading", () => { beforeEach(() => { isOpen = true; + isLoading = false; }); it("renders the popup content inside a PF/Modal", async () => { @@ -79,6 +85,14 @@ describe("Popup", () => { within(dialog).getByText("The Popup Content"); }); + it("does not display a progress message", async () => { + installerRender(); + + const dialog = await screen.findByRole("dialog"); + + expect(within(dialog).queryByText(loadingText)).toBeNull(); + }); + it("renders the popup actions inside a PF/Modal footer", async () => { installerRender(); @@ -92,6 +106,22 @@ describe("Popup", () => { within(footer).getByText("Cancel"); }); }); + + describe("when it is open and loading", () => { + beforeEach(() => { + isOpen = true; + isLoading = true; + }); + + it("displays progress message instead of the content", async () => { + installerRender(); + + const dialog = await screen.findByRole("dialog"); + + expect(within(dialog).queryByText("The Popup Content")).toBeNull(); + within(dialog).getByText(loadingText); + }); + }); }); describe("Popup.PrimaryAction", () => { From 2f954fa85553772270928e150d57fc7edd87e891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ladislav=20Slez=C3=A1k?= Date: Thu, 2 May 2024 15:29:38 +0200 Subject: [PATCH 13/13] Improve unit test --- .../storage/InstallationDeviceField.test.jsx | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/web/src/components/storage/InstallationDeviceField.test.jsx b/web/src/components/storage/InstallationDeviceField.test.jsx index 2b3d1fc234..13ffa9a570 100644 --- a/web/src/components/storage/InstallationDeviceField.test.jsx +++ b/web/src/components/storage/InstallationDeviceField.test.jsx @@ -26,6 +26,15 @@ import { screen, within } from "@testing-library/react"; import { installerRender } from "~/test-utils"; import InstallationDeviceField from "~/components/storage/InstallationDeviceField"; +jest.mock("@patternfly/react-core", () => { + const original = jest.requireActual("@patternfly/react-core"); + + return { + ...original, + Skeleton: () =>
PF-Skeleton
+ }; +}); + /** * @typedef {import ("~/components/storage/InstallationDeviceField").InstallationDeviceFieldProps} InstallationDeviceFieldProps * @typedef {import ("~/client/storage").StorageDevice} StorageDevice @@ -97,10 +106,10 @@ describe("when set as loading", () => { }); it("renders a loading hint", () => { - const { container } = installerRender(); + installerRender(); // a PF skeleton is displayed - expect(container.querySelector("div.pf-v5-c-skeleton")).toBeVisible(); + screen.getByText("PF-Skeleton"); }); });