Skip to content

Storage: show skeletons only when needed #1171

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.

Sign up for GitHub

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

Merged
lslezak merged 13 commits into from
May 2, 2024
Merged

Storage: show skeletons only when needed #1171

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
837a880
Storage: show skeletons only when needed
lslezak Apr 26, 2024
d093d36
More comments
lslezak Apr 26, 2024
f535c4d
Simplify screen reader texts
lslezak Apr 29, 2024
358e04a
Always refresh the proposal result
lslezak Apr 29, 2024
30f8d7a
Update test
lslezak Apr 29, 2024
4583eaf
Code review fixes
lslezak Apr 29, 2024
83eccdf
Review fixes
lslezak Apr 30, 2024
a143e22
Revert the <If> change
lslezak Apr 30, 2024
5d231ca
Add `isLoading` property to the <Popup> component
lslezak Apr 30, 2024
bc2cd45
Make the progress message configurable
lslezak Apr 30, 2024
db04da1
Reset the values after loading is finished
lslezak May 2, 2024
da9ec7d
Update unit test
lslezak May 2, 2024
2f954fa
Improve unit test
lslezak May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions web/src/components/core/If.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

@lslezak lslezak Apr 30, 2024

Choose a reason for hiding this comment

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

This allows lazy evaluation of the branches. The problem is that <If> evaluates both branches and then returns one of them.

So for example this

<If condition={data === undefined} then={<Loading/>} else={<Foo foo={data.foo}>} />

crashes when data is undefined because it evaluates the else branch too early.

With this improvement you can pass a function which can be evaluated only when the result is needed. (Passing functions is quite common in Javascript to allow the lazy evaluation.)

Then the fix is trivial, just add () => before the value:

<If condition={data === undefined} then={<Loading/>} else={() => <Foo foo={data.foo}>} />

@dgdavid are you OK with this change?

I'll add some example into the <If> documentation if you agree with this improvement.

Copy link
Contributor

@dgdavid dgdavid Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @lslezak

Let's say that I have no strong opinion about it. At first sight, it looks reasonable to prevent not needed evaluations. But on the other hand it's weird to me to accept a function as content of the branches. IMHO it should be limited to ReactNode (Although technically a component is a function too).

That said, I fully understand the proposal looking at the code written at https://github.com/openSUSE/agama/pull/1171/files#diff-994f1f4ff00fecec1797a2aaa799dbbeddc4e8fb2c1ae666fedbb95b7c499770R155-R178

  <If
        condition={isLoading}
        then={
          <Loading text={_("Loading data...")} />
        }
        else={
          () =>
            <Form id="space-policy-form" onSubmit={onSubmit}>
              <SpacePolicyPicker currentPolicy={policy} onChange={setPolicy} />
              <If
                condition={devices.length > 0}
                then={
                  <SpaceActionsTable
                    devices={devices}
                    expandedDevices={expandedDevices}
                    deviceAction={deviceAction}
                    isActionDisabled={policy.id !== "custom"}
                    onActionChange={changeActions}
                  />
                }
              />
            </Form>
        }
      />

But I would go for a different implementation instead. For me, what is in the else should be a component that React should be able to understand. Let's say <SpacePolicyForm />. Then, the component should be aware about what it needs for return either, an output or nothing.

const SpacePolicyForm = ({ prop1, prop2 }) => {
  if (!prop1 || !prop2) return;

  ...

  return (
    <div><b>{prop1}:</b>{prop2}</div>
  );
}

I.e., the key for me is to write more robust components.

Of course, I might be wrong, so maybe another view here could help to decide what way to go.

Copy link
Contributor

@joseivanlopez joseivanlopez Apr 30, 2024

Choose a reason for hiding this comment

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

I was assuming the else branch is not evaluated if the condition is true. So, probably, I have written unsafe code.

Copy link
Contributor

@dgdavid dgdavid Apr 30, 2024

Choose a reason for hiding this comment

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

BTW, as far as I can see I think that this set of changes reveals another change we could address: to add the isLoading prop to core/Popup and make it show the spinner or its children based on the prop value. That way, we could reduce the <If condition={isLoading} then={... repetition and have more clean WhateverDialog.jsx components.

Copy link
Contributor Author

@lslezak lslezak Apr 30, 2024

Choose a reason for hiding this comment

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

After reading this I decided to revert the <If> change. It seems it is not safe and it can have some nasty side effects in some cases.

Copy link
Contributor

@imobachgs imobachgs May 2, 2024

Choose a reason for hiding this comment

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

@joseivanlopez Actually I do not think you have written "unsafe" code. The Form itself is not executed (@dgdavid and me has done some tests with another example). The problem is the data.foo when data is undefined (that's the code that gets executed).

}
10 changes: 10 additions & 0 deletions web/src/components/core/If.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ describe("If", () => {
it("renders content given in 'then' prop", () => {
plainRender(<If condition={3 < 6} then="Hello World!" else="Goodbye World!" />);

screen.getByText("Hello World!");
});
it("renders result of function given in 'then' prop", () => {
plainRender(<If condition={3 < 6} then={() => "Hello World!"} else={() => "Goodbye World!"} />);

screen.getByText("Hello World!");
});
});
Expand All @@ -48,6 +53,11 @@ describe("If", () => {
it("renders content given in 'else' prop", () => {
plainRender(<If condition={6 < 3} then="Hello World!" else="Goodbye World!" />);

screen.getByText("Goodbye World!");
});
it("renders result of function given in 'else' prop", () => {
plainRender(<If condition={6 < 3} then={() => "Hello World!"} else={() => "Goodbye World!"} />);

screen.getByText("Goodbye World!");
});
});
Expand Down
4 changes: 2 additions & 2 deletions web/src/components/storage/BootConfigField.jsx
View file
Open in desktop
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
133 changes: 75 additions & 58 deletions web/src/components/storage/DeviceSelectionDialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ 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 } from "~/components/core";
import { DeviceSelectorTable } from "~/components/storage";
import { noop } from "~/utils";
import { Loading } from "~/components/layout";

/**
* @typedef {import ("~/client/storage").ProposalTarget} ProposalTarget
Expand All @@ -52,6 +53,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 +69,7 @@ export default function DeviceSelectionDialog({
targetPVDevices: defaultPVDevices,
devices,
isOpen,
isLoading,
onCancel = noop,
onAccept = noop,
...props
Expand Down Expand Up @@ -95,6 +98,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 @@ -118,63 +126,72 @@ devices.").split(/[[\]]/);
inlineSize="large"
{...props}
>
<Form id="target-form" onSubmit={onSubmit}>
<Panels className="stack">
<Panels.Options data-variant="buttons">
<Panels.Option
id={SELECT_DISK_ID}
name={OPTIONS_NAME}
isSelected={isTargetDisk}
onChange={selectTargetDisk}
controls={SELECT_DISK_PANEL_ID}
>
{_("Select a disk")}
</Panels.Option>
<Panels.Option
id={CREATE_LVM_ID}
name={OPTIONS_NAME}
isSelected={isTargetNewLvmVg}
onChange={selectTargetNewLvmVG}
controls={CREATE_LVM_PANEL_ID}
>
{_("Create an LVM Volume Group")}
</Panels.Option>
</Panels.Options>

<Panels.Panel id={SELECT_DISK_PANEL_ID} isExpanded={isTargetDisk}>
{msgStart1}
<b>{msgBold1}</b>
{msgEnd1}

<DeviceSelectorTable
aria-label={_("Device selector for target disk")}
devices={devices}
selected={[targetDevice]}
itemChildren={deviceChildren}
itemSelectable={isDeviceSelectable}
onSelectionChange={selectTargetDevice}
variant="compact"
/>
</Panels.Panel>

<Panels.Panel id={CREATE_LVM_PANEL_ID} isExpanded={isTargetNewLvmVg} className="stack">
{msgStart2}
<b>{msgBold2}</b>
{msgEnd2}

<DeviceSelectorTable
aria-label={_("Device selector for new LVM volume group")}
isMultiple
devices={devices}
selected={targetPVDevices}
itemChildren={deviceChildren}
itemSelectable={isDeviceSelectable}
onSelectionChange={setTargetPVDevices}
variant="compact"
/>
</Panels.Panel>
</Panels>
</Form>
<If
condition={isLoading}
then={
<Loading text={_("Loading data...")} />
}
else={
<Form id="target-form" onSubmit={onSubmit}>
<Panels className="stack">
<Panels.Options data-variant="buttons">
<Panels.Option
id={SELECT_DISK_ID}
name={OPTIONS_NAME}
isSelected={isTargetDisk}
onChange={selectTargetDisk}
controls={SELECT_DISK_PANEL_ID}
>
{_("Select a disk")}
</Panels.Option>
<Panels.Option
id={CREATE_LVM_ID}
name={OPTIONS_NAME}
isSelected={isTargetNewLvmVg}
onChange={selectTargetNewLvmVG}
controls={CREATE_LVM_PANEL_ID}
>
{_("Create an LVM Volume Group")}
</Panels.Option>
</Panels.Options>

<Panels.Panel id={SELECT_DISK_PANEL_ID} isExpanded={isTargetDisk}>
{msgStart1}
<b>{msgBold1}</b>
{msgEnd1}

<DeviceSelectorTable
aria-label={_("Device selector for target disk")}
devices={devices}
selected={[targetDevice]}
itemChildren={deviceChildren}
itemSelectable={isDeviceSelectable}
onSelectionChange={selectTargetDevice}
variant="compact"
/>
</Panels.Panel>

<Panels.Panel id={CREATE_LVM_PANEL_ID} isExpanded={isTargetNewLvmVg} className="stack">
{msgStart2}
<b>{msgBold2}</b>
{msgEnd2}

<DeviceSelectorTable
aria-label={_("Device selector for new LVM volume group")}
isMultiple
devices={devices}
selected={targetPVDevices}
itemChildren={deviceChildren}
itemSelectable={isDeviceSelectable}
onSelectionChange={setTargetPVDevices}
variant="compact"
/>
</Panels.Panel>
</Panels>
</Form>
}
/>

<Popup.Actions>
<Popup.Confirm form="target-form" type="submit" isDisabled={isAcceptDisabled()} />
<Popup.Cancel onClick={onCancel} />
Expand Down
3 changes: 2 additions & 1 deletion web/src/components/storage/InstallationDeviceField.jsx
View file
Open in desktop
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
View file
Open in desktop
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

@dgdavid dgdavid May 2, 2024

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

@dgdavid dgdavid May 2, 2024

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
View file
Open in desktop
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
View file
Open in desktop
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