Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage: show skeletons only when needed #1171

Merged
merged 13 commits into from
May 2, 2024
5 changes: 4 additions & 1 deletion web/src/components/core/Popup.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<ModalProps, "variant" | "size"> & PopupBaseProps} PopupProps
*
* @param {PopupProps} props
*/
const Popup = ({
isOpen = false,
isLoading = false,
showClose = false,
inlineSize = "medium",
blockSize = "auto",
Expand All @@ -214,7 +217,7 @@ const Popup = ({
actions={actions}
className={`${className} block-size-${blockSize} inline-size-${inlineSize}`.trim()}
>
{content}
{isLoading ? <Loading text={_("Loading data...")} /> : content}
dgdavid marked this conversation as resolved.
Show resolved Hide resolved
</Modal>
);
};
Expand Down
4 changes: 2 additions & 2 deletions web/src/components/storage/BootConfigField.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ export default function BootConfigField({
onChange({ configureBoot, bootDevice });
};

if (isLoading) {
return <Skeleton screenreaderText={_("Waiting for information about boot config")} width="75%" />;
if (isLoading && configureBoot === undefined) {
return <Skeleton width="75%" />;
}

let value;
Expand Down
9 changes: 9 additions & 0 deletions web/src/components/storage/DeviceSelectionDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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]
*
Expand All @@ -67,6 +68,7 @@ export default function DeviceSelectionDialog({
targetPVDevices: defaultPVDevices,
devices,
isOpen,
isLoading,
onCancel = noop,
onAccept = noop,
...props
Expand Down Expand Up @@ -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
Expand All @@ -114,6 +121,7 @@ devices.").split(/[[\]]/);
<Popup
title={_("Device for installing the system")}
isOpen={isOpen}
isLoading={isLoading}
blockSize="large"
inlineSize="large"
{...props}
Expand Down Expand Up @@ -175,6 +183,7 @@ devices.").split(/[[\]]/);
</Panels.Panel>
</Panels>
</Form>

<Popup.Actions>
<Popup.Confirm form="target-form" type="submit" isDisabled={isAcceptDisabled()} />
<Popup.Cancel onClick={onCancel} />
Expand Down
1 change: 1 addition & 0 deletions web/src/components/storage/EncryptionField.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export default function EncryptionField({
method={method}
methods={methods}
isOpen={isDialogOpen}
isLoading={isLoading}
onCancel={closeDialog}
onAccept={onAccept}
/>
Expand Down
11 changes: 10 additions & 1 deletion web/src/components/storage/EncryptionSettingsDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -63,6 +64,7 @@ export default function EncryptionSettingsDialog({
method: methodProp,
methods,
isOpen = false,
isLoading = false,
onCancel,
onAccept
}) {
Expand All @@ -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]);
Expand All @@ -91,7 +100,7 @@ export default function EncryptionSettingsDialog({
};

return (
<Popup title={DIALOG_TITLE} description={DIALOG_DESCRIPTION} isOpen={isOpen}>
<Popup title={DIALOG_TITLE} description={DIALOG_DESCRIPTION} isOpen={isOpen} isLoading={isLoading}>
<SwitchField
highlightContent
isChecked={isEnabled}
Expand Down
3 changes: 2 additions & 1 deletion web/src/components/storage/InstallationDeviceField.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export default function InstallationDeviceField({

let value;
if (isLoading || !target)
value = <Skeleton screenreaderText={_("Waiting for information about selected device")} width="25%" />;
value = <Skeleton width="25%" />;
else
value = targetValue(target, targetDevice, targetPVDevices);

Expand All @@ -127,6 +127,7 @@ export default function InstallationDeviceField({
then={
<DeviceSelectionDialog
isOpen
isLoading={isLoading}
target={target}
targetDevice={targetDevice}
targetPVDevices={targetPVDevices}
Expand Down
6 changes: 4 additions & 2 deletions web/src/components/storage/InstallationDeviceField.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,10 @@ describe("when set as loading", () => {
});

it("renders a loading hint", () => {
installerRender(<InstallationDeviceField {...props} />);
screen.getByText("Waiting for information about selected device");
const { container } = installerRender(<InstallationDeviceField {...props} />);

// a PF skeleton is displayed
expect(container.querySelector("div.pf-v5-c-skeleton")).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: We use to mock the PF/Skeleton and look for the mocked content instead, which is somehow more aligned with RTL principles (to look for what the user see, not how it's styled).

https://github.com/openSUSE/agama/blob/7e43d5883ef894000194c841c2e547ca3fa81455/web/src/components/storage/ProposalPage.test.jsx#L40-L48

But feel free to keep the test as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also more robust in the sense that no matter if in the next PF update they change the HTML node for wrapping the content or the CSS classes used for styling it. The latest will happens as soon as we migrate to PFv6.

});
});

Expand Down
38 changes: 33 additions & 5 deletions web/src/components/storage/ProposalPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import { IDLE } from "~/client/status";

const initialState = {
loading: true,
// which UI item is being changed by user
changing: undefined,
availableDevices: [],
volumeTemplates: [],
encryptionMethods: [],
Expand All @@ -52,7 +54,8 @@ const reducer = (state, action) => {
}

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": {
Expand All @@ -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": {
Expand All @@ -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.
// the ProposalResultSection is refreshed always
InstallationDeviceField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.POLICY, CHANGING.VOLUMES],
PartitionsField: [CHANGING.ENCRYPTION, CHANGING.POLICY],
SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.TARGET],
};

export default function ProposalPage() {
const { storage: client } = useInstallerClient();
const { cancellablePromise } = useCancellablePromise();
Expand Down Expand Up @@ -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);
};

Expand All @@ -236,6 +263,7 @@ export default function ProposalPage() {
settings={state.settings}
onChange={changeSettings}
isLoading={state.loading}
changing={state.changing}
/>
<ProposalResultSection
system={state.system}
Expand Down
3 changes: 2 additions & 1 deletion web/src/components/storage/ProposalResultSection.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ const DevicesTreeTable = ({ devicesManager }) => {
const ResultSkeleton = () => {
return (
<>
<Skeleton width="80%" />
<Skeleton screenreaderText={_("Waiting for information about storage configuration")} width="80%" />
<Skeleton width="65%" />
<Skeleton width="70%" />
</>
Expand Down Expand Up @@ -251,6 +251,7 @@ 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 = [],
Expand Down
62 changes: 43 additions & 19 deletions web/src/components/storage/ProposalSettingsSection.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { _ } from "~/i18n";
import { compact } from "~/utils";
import { Section } from "~/components/core";
import { SPACE_POLICIES } from "~/components/storage/utils";
import { CHANGING, NOT_AFFECTED } from "~/components/storage/ProposalPage";
import EncryptionField from "~/components/storage/EncryptionField";
import InstallationDeviceField from "~/components/storage/InstallationDeviceField";
import PartitionsField from "~/components/storage/PartitionsField";
Expand All @@ -40,6 +41,18 @@ import SpacePolicyField from "~/components/storage/SpacePolicyField";
* @typedef {import ("~/client/storage").Volume} Volume
*/

/**
* A helper function to decide whether to show the progress skeletons or not
* 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
*/
const showSkeleton = (loading, component, changing) => {
return loading && !NOT_AFFECTED[component].includes(changing);
};

/**
* Section for editing the proposal settings
* @component
Expand All @@ -50,7 +63,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
*/
Expand All @@ -60,41 +74,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
}
);
};

/**
Expand All @@ -120,7 +144,7 @@ export default function ProposalSettingsSection({
targetDevice={targetDevice}
targetPVDevices={targetPVDevices}
devices={availableDevices}
isLoading={isLoading}
isLoading={showSkeleton(isLoading, "InstallationDeviceField", changing)}
onChange={changeTarget}
/>
<EncryptionField
Expand All @@ -139,15 +163,15 @@ export default function ProposalSettingsSection({
configureBoot={settings.configureBoot}
bootDevice={bootDevice}
defaultBootDevice={defaultBootDevice}
isLoading={isLoading || settings.volumes === undefined}
isLoading={showSkeleton(isLoading, "PartitionsField", changing) || settings.volumes === undefined}
onVolumesChange={changeVolumes}
onBootChange={changeBoot}
/>
<SpacePolicyField
policy={spacePolicy}
actions={spaceActions}
devices={installationDevices}
isLoading={isLoading}
isLoading={showSkeleton(isLoading, "SpacePolicyField", changing)}
onChange={changeSpacePolicy}
/>
</Section>
Expand Down
Loading
Loading