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 5 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
130 changes: 72 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,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";

Expand All @@ -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 @@ -118,63 +125,70 @@ 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={ <SectionSkeleton numRows={6} /> }
Copy link
Contributor

@ancorgs ancorgs Apr 29, 2024

Choose a reason for hiding this comment

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

Honestly, this skeleton here looks really weird.

As commented during the planning meeting of the card and again today during review, the goal of the skeletons is to "reserve space" in the UI for the future content, so things don't jump up and down as that content is loaded into its place.

In case of a pop-up that is going to load at once as soon the content arrives, that multi-line skeleton looks like some kind of render error. I expected the pop-up to open kind of blank but with some "loading" message and/or a spinner (I personally expected the same three circles we have when loading the whole UI).

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],
Copy link
Contributor

@ancorgs ancorgs Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this include CHANGING.POLICY? I don't see how changing the find space policy can affect the list of volumes.

SpacePolicyField: [CHANGING.ENCRYPTION, CHANGING.BOOT, CHANGING.VOLUMES, CHANGING.TARGET],
Copy link
Contributor

@ancorgs ancorgs Apr 29, 2024

Choose a reason for hiding this comment

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

Just a comment about this. Being fully strict, the fact is that the content of the sentence displayed right after "Find Space" can actually be affected by changes in the target disk(s), in the booting disk and in the list of volumes.

For example, the sentence can initially be "deleting all content of the installation disk". But then I change the target and decide to install on an LVM over sdb and sdc. Then the sentence changes to "deleting all content of the 2 selected disks". Then I decide that, in addition to that, the system will boot from sda so the sentence changes to "deleting all content of the 3 selected disks". And for extra fun I decide to allocate /home as a new partition at sdd. Then the sentence changes again to "deleting all content of the 4 selected disks".

I don't know if those changes in the sentence really deserve a skeleton. I just wanted to mention there is SOME influence and as such it should be documented next to the code.

};

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