Storage: show skeletons only when needed (#1171)

## Problem

- The loading skeletons are displayed always, even when not needed
- The installation device selection popup is empty when clicked quickly
before loading all data

## Solution

### Loading Skeletons

- Pass the name of the component which is changed by user to parent
- Send back the changing component to all children
- There is mapping which defined whether a child needs refresh when some
setting is being changed

### Installation Device Progress

- Use the sent data when when the stored values are `undefined`
- Show a simple skeleton

### screenreaderText Properties

- I have removed all `screenreaderText` properties for skeletons and
keep it only for the result section
- The reason is that it does not make sense to notify users about 5
values in progress, it usually takes about 2 seconds to refresh the data
so the screen reader would not have enough time to read all messages
- Use it only the result section which is refreshed always, moreover the
data for the other values will be read together with the result data,
they will always take the same time

## Questions

- Do we need more granularity with changing values? I.e. if the Btrfs
snapshots are enabled/disabled then the partition list below is refresh
as well. (IMHO fine to solve this later in a separate card / PR.)
- Better progress for the installation device loading? (Note: do not
overengineer here, the progress is displayed only when you click the
link very quickly after opening the storage page, in most cases it won't
be used at all.)

## Testing

- Tested manually

## Screenshots

When enabling/disabling the Btrfs snapshots all data was refreshed, now
the installation device and the space policy does not show the progress
skeletons.

| Original | Fixed |
|---|---|
|
![storage_skeletons_broken](https://github.com/openSUSE/agama/assets/907998/9833814b-ee02-42c3-be7f-089f4b832cba)
|
![storage_skeletons_fixed](https://github.com/openSUSE/agama/assets/907998/0c695214-3804-42ee-b14d-4af64d74fdd3)
|

Another fix was empty installation device screen when the "Installation
device" link was clicked quickly, before loading the data finished.

| Original | Fixed |
|---|---|
|
![storage_loading_progress_broken](https://github.com/openSUSE/agama/assets/907998/70b7fd46-63f5-44ec-b5b7-63a4de0ffa81)
|
![storage_loading_progress2](https://github.com/openSUSE/agama/assets/907998/c7989b53-b116-4264-92e3-f48cc69d671f)
|

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,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",
@@ -214,7 +220,7 @@ const Popup = ({
       actions={actions}
       className={`${className} block-size-${blockSize} inline-size-${inlineSize}`.trim()}
     >
-      {content}
+      {isLoading ? <Loading text={loadingText} /> : content}
     </Modal>
   );
 };

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) => {
     <Popup
       title="Testing Popup component"
       isOpen={isOpen}
+      isLoading={isLoading}
+      loadingText={loadingText}
       {...props}
     >
       <p>The Popup Content</p>
@@ -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(<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 />);
 
@@ -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", () => {

web/src/components/storage/BootConfigField.jsx

@@ -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;

web/src/components/storage/DeviceSelectionDialog.jsx

@@ -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
@@ -114,6 +121,7 @@ devices.").split(/[[\]]/);
     <Popup
       title={_("Device for installing the system")}
       isOpen={isOpen}
+      isLoading={isLoading}
       blockSize="large"
       inlineSize="large"
       {...props}
@@ -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} />

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}
           />

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
 }) {
@@ -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]);
@@ -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}

web/src/components/storage/InstallationDeviceField.jsx

@@ -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);
 
@@ -127,6 +127,7 @@ export default function InstallationDeviceField({
         then={
           <DeviceSelectionDialog
             isOpen
+            isLoading={isLoading}
             target={target}
             targetDevice={targetDevice}
             targetPVDevices={targetPVDevices}

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: () => <div>PF-Skeleton</div>
+  };
+});
+
 /**
  * @typedef {import ("~/components/storage/InstallationDeviceField").InstallationDeviceFieldProps} InstallationDeviceFieldProps
  * @typedef {import ("~/client/storage").StorageDevice} StorageDevice
@@ -98,7 +107,9 @@ describe("when set as loading", () => {
 
   it("renders a loading hint", () => {
     installerRender(<InstallationDeviceField {...props} />);
-    screen.getByText("Waiting for information about selected device");
+
+    // a PF skeleton is displayed
+    screen.getByText("PF-Skeleton");
   });
 });
 

web/src/components/storage/ProposalPage.jsx

@@ -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: [],
@@ -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": {
@@ -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.
+  // 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();
@@ -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}
       />
       <ProposalResultSection
         system={state.system}

web/src/components/storage/ProposalResultSection.jsx

@@ -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%" />
     </>
@@ -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 = [],