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
8 changes: 7 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,17 @@ 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
* @property {string} [loadingText="Loading data..."] - Text displayed when `isLoading` is set to `true`
* @typedef {Omit<ModalProps, "variant" | "size"> & PopupBaseProps} PopupProps
*
* @param {PopupProps} props
*/
const Popup = ({
isOpen = false,
isLoading = false,
// TRANSLATORS: progress message
loadingText = _("Loading data..."),
showClose = false,
inlineSize = "medium",
blockSize = "auto",
Expand All @@ -214,7 +220,7 @@ const Popup = ({
actions={actions}
className={`${className} block-size-${blockSize} inline-size-${inlineSize}`.trim()}
>
{content}
{isLoading ? <Loading text={loadingText} /> : content}
</Modal>
);
};
Expand Down
32 changes: 31 additions & 1 deletion web/src/components/core/Popup.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -39,6 +41,8 @@ const TestingPopup = (props) => {
<Popup
title="Testing Popup component"
isOpen={isOpen}
isLoading={isLoading}
loadingText={loadingText}
{...props}
>
<p>The Popup Content</p>
Expand All @@ -55,6 +59,7 @@ describe("Popup", () => {
describe("when it is not open", () => {
beforeEach(() => {
isOpen = false;
isLoading = false;
});

it("renders nothing", async () => {
Expand All @@ -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 () => {
Expand All @@ -79,6 +85,14 @@ describe("Popup", () => {
within(dialog).getByText("The Popup Content");
});

it("does not display a progress message", async () => {
installerRender(<TestingPopup />);

const dialog = await screen.findByRole("dialog");

expect(within(dialog).queryByText(loadingText)).toBeNull();
});

it("renders the popup actions inside a PF/Modal footer", async () => {
installerRender(<TestingPopup />);

Expand All @@ -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(<TestingPopup />);

const dialog = await screen.findByRole("dialog");

expect(within(dialog).queryByText("The Popup Content")).toBeNull();
within(dialog).getByText(loadingText);
});
});
});

describe("Popup.PrimaryAction", () => {
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
17 changes: 16 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 @@ -71,8 +73,21 @@ 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";

// 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(() => {
setValidSettings(!isEnabled || (password.length > 0 && passwordsMatch));
}, [isEnabled, password, passwordsMatch]);
Expand All @@ -91,7 +106,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
Loading
Loading