web: Use meaningful icons in Storage proposal page (#1152)

During the presentation of #1138, it was seen that the _shadow_ icon
chosen for some of the actions opening a dialog created confusion in
some attendees when other no-iconized actions opened a dialog too.

Somehow, it was forgotten one of the principles followed in the Agama UI
until now: an action (link, button, button dressed up as a link, menu
item, etc) should be easily recognizable by users as an element they can
interact with in the way they use to do it. I.e., it's not needed to
identify if the action opens a dialog, navigates to another page, to
another section of the same page, opens a menu, shows or hides a
sidebar, etc, since the same action that today opens a dialog in the
future might navigate to another page instead.

Proposed changes here fix the issue by replacing these icons with ones
that represent the concept of the action or by none when it would hamper
more than help. **Keep in mind that icons are merely cosmetic aids for
sight people, reason why the meaning of the actions must not be
determined by them**. The UI must still be meaningful if, for whatever
reason, icons disappear.

Of course, the ones proposed here can be changed at any point when
better suggestions arise. **But remember that at this moment we're
mainly relying on https://fonts.google.com/icons**. We can change our
mind in the future, but choosing a consistent library of icons instead
of grabbing them from here and there. For example, [Lucid
icons](https://lucide.dev/icons/) could be a good candidate for a
library replacement, but in a future **PLEASE**.

web/package/cockpit-agama.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Tue Apr 16 11:15:13 UTC 2024 - David Diaz <[email protected]>
+
+- Use better icons in storage proposal page
+  (gh#openSUSE/agama#1152).
+
 -------------------------------------------------------------------
 Mon Apr 15 10:52:14 UTC 2024 - David Diaz <[email protected]>
 

web/src/components/core/Field.jsx

@@ -82,13 +82,6 @@ const Field = ({
   );
 };
 
-/**
- * @param {Omit<FieldProps, 'icon'>} props
- */
-const SettingsField = ({ ...props }) => {
-  return <Field {...props} icon="shadow" />;
-};
-
 /**
  * @param {Omit<FieldProps, 'icon'> & {isChecked: boolean, highlightContent?: boolean}} props
  */
@@ -108,14 +101,15 @@ const SwitchField = ({ isChecked = false, highlightContent = false, ...props })
 };
 
 /**
- * @param {Omit<FieldProps, 'icon'> & {isExpanded: boolean}} props
+ * @param {FieldProps & {isExpanded: boolean}} props
  */
-const ExpandableField = ({ isExpanded, ...props }) => {
+const ExpandableField = ({ label, isExpanded, ...props }) => {
   const iconName = isExpanded ? "collapse_all" : "expand_all";
   const className = isExpanded ? "expanded" : "collapsed";
+  const labelWithIcon = <>{label} <Icon name={iconName} size="xs" /></>;
 
-  return <Field {...props} icon={iconName} className={className} />;
+  return <Field {...props} label={labelWithIcon} className={className} />;
 };
 
 export default Field;
-export { ExpandableField, SettingsField, SwitchField };
+export { ExpandableField, SwitchField };

web/src/components/core/Field.test.jsx

@@ -22,7 +22,7 @@
 import React from "react";
 import { screen } from "@testing-library/react";
 import { plainRender } from "~/test-utils";
-import { Field, ExpandableField, SettingsField, SwitchField } from "~/components/core";
+import { Field, ExpandableField, SwitchField } from "~/components/core";
 
 const onClick = jest.fn();
 
@@ -64,17 +64,6 @@ describe("Field", () => {
   });
 });
 
-describe("SettingsField", () => {
-  it("uses the 'shadow' icon", () => {
-    const { container } = plainRender(
-      // Trying to set other icon, although typechecking should catch it.
-      <SettingsField icon="edit" label="Theme" value="dark" onClick={onClick} />
-    );
-    const icon = container.querySelector("button > svg");
-    expect(icon).toHaveAttribute("data-icon-name", "shadow");
-  });
-});
-
 describe("SwitchField", () => {
   it("sets button role to switch", () => {
     plainRender(<SwitchField label="Zoom" value="enabled" isChecked />);
@@ -111,19 +100,15 @@ describe("SwitchField", () => {
 });
 
 describe("ExpandableField", () => {
-  it("uses the 'collapse_all' icon when isExpanded", () => {
-    const { container } = plainRender(
-      <ExpandableField label="More settings" isExpanded />
-    );
-    const icon = container.querySelector("button > svg");
-    expect(icon).toHaveAttribute("data-icon-name", "collapse_all");
+  it("uses 'expanded' as className prop value when isExpanded", () => {
+    const { container } = plainRender(<ExpandableField label="More settings" isExpanded />);
+    const field = container.querySelector("[data-type='agama/field']");
+    expect(field.classList.contains("expanded")).toBe(true);
   });
 
-  it("uses the 'expand_all' icon when not isExpanded", () => {
-    const { container } = plainRender(
-      <ExpandableField label="More settings" />
-    );
-    const icon = container.querySelector("button > svg");
-    expect(icon).toHaveAttribute("data-icon-name", "expand_all");
+  it("uses 'collapsed' as className prop value when isExpanded", () => {
+    const { container } = plainRender(<ExpandableField label="More settings" />);
+    const field = container.querySelector("[data-type='agama/field']");
+    expect(field.classList.contains("collapsed")).toBe(true);
   });
 });

web/src/components/core/index.js

@@ -63,4 +63,4 @@ export { default as Tag } from "./Tag";
 export { default as TreeTable } from "./TreeTable";
 export { default as ControlledPanels } from "./ControlledPanels";
 export { default as Field } from "./Field";
-export { ExpandableField, SettingsField, SwitchField } from "./Field";
+export { ExpandableField, SwitchField } from "./Field";

web/src/components/layout/Icon.jsx

@@ -41,6 +41,7 @@ import ExpandMore from "@icons/expand_more.svg?component";
 import Feedback from "@icons/feedback.svg?component";
 import Folder from "@icons/folder.svg?component";
 import FolderOff from "@icons/folder_off.svg?component";
+import FrameInspect from "@icons/frame_inspect.svg?component";
 import Globe from "@icons/globe.svg?component";
 import HardDrive from "@icons/hard_drive.svg?component";
 import Help from "@icons/help.svg?component";
@@ -63,6 +64,7 @@ import SettingsApplications from "@icons/settings_applications.svg?component";
 import SettingsEthernet from "@icons/settings_ethernet.svg?component";
 import SettingsFill from "@icons/settings-fill.svg?component";
 import Shadow from "@icons/shadow.svg?component";
+import ShieldLock from "@icons/shield_lock.svg?component";
 import SignalCellularAlt from "@icons/signal_cellular_alt.svg?component";
 import Storage from "@icons/storage.svg?component";
 import Sync from "@icons/sync.svg?component";
@@ -109,6 +111,7 @@ const icons = {
   feedback: Feedback,
   folder: Folder,
   folder_off: FolderOff,
+  frame_inspect: FrameInspect,
   globe: Globe,
   hard_drive: HardDrive,
   help: Help,
@@ -132,6 +135,7 @@ const icons = {
   settings_applications: SettingsApplications,
   settings_ethernet: SettingsEthernet,
   shadow: Shadow,
+  shield_lock: ShieldLock,
   signal_cellular_alt: SignalCellularAlt,
   storage: Storage,
   sync: Sync,

web/src/components/storage/BootConfigField.jsx

@@ -47,7 +47,7 @@ const Button = ({ isBold = false, onClick }) => {
 
   return (
     <button onClick={onClick} className="inline-flex-button">
-      {isBold ? <b>{text}</b> : text} <Icon name="shadow" size="xxs" />
+      {isBold ? <b>{text}</b> : text}
     </button>
   );
 };
@@ -68,7 +68,7 @@ const Button = ({ isBold = false, onClick }) => {
  * @property {boolean} configureBoot
  * @property {StorageDevice} bootDevice
  */
-export default function BootConfigField ({
+export default function BootConfigField({
   configureBoot,
   bootDevice,
   defaultBootDevice,
@@ -104,7 +104,7 @@ export default function BootConfigField ({
 
   return (
     <div>
-      { value } <Button onClick={openDialog} isBold={!configureBoot} />
+      {value} <Button onClick={openDialog} isBold={!configureBoot} />
       <If
         condition={isDialogOpen}
         then={

web/src/components/storage/EncryptionField.jsx

@@ -25,7 +25,7 @@ import React, { useCallback, useEffect, useState } from "react";
 import { Skeleton } from "@patternfly/react-core";
 import { _ } from "~/i18n";
 import { noop } from "~/utils";
-import { If, SettingsField } from "~/components/core";
+import { If, Field } from "~/components/core";
 import { EncryptionMethods } from "~/client/storage";
 import EncryptionSettingsDialog from "~/components/storage/EncryptionSettingsDialog";
 
@@ -91,7 +91,8 @@ export default function EncryptionField({
   };
 
   return (
-    <SettingsField
+    <Field
+      icon="shield_lock"
       label={LABEL}
       description={DESCRIPTION}
       value={isLoading ? VALUES.loading : VALUES[isEnabled ? method : "disabled"]}
@@ -110,6 +111,6 @@ export default function EncryptionField({
           />
         }
       />
-    </SettingsField>
+    </Field>
   );
 }

web/src/components/storage/InstallationDeviceField.jsx

@@ -27,7 +27,7 @@ import { Skeleton } from "@patternfly/react-core";
 import { _ } from "~/i18n";
 import { DeviceSelectionDialog, ProposalPageMenu } from "~/components/storage";
 import { deviceLabel } from '~/components/storage/utils';
-import { If, SettingsField } from "~/components/core";
+import { If, Field } from "~/components/core";
 import { sprintf } from "sprintf-js";
 
 /**
@@ -114,7 +114,8 @@ export default function InstallationDeviceField({
     value = targetValue(target, targetDevice, targetPVDevices);
 
   return (
-    <SettingsField
+    <Field
+      icon="hard_drive"
       label={LABEL}
       value={value}
       description={DESCRIPTION}
@@ -135,6 +136,6 @@ export default function InstallationDeviceField({
           />
         }
       />
-    </SettingsField>
+    </Field>
   );
 }

web/src/components/storage/PartitionsField.jsx

@@ -698,6 +698,7 @@ export default function PartitionsField({
 
   return (
     <ExpandableField
+      icon="storage"
       isExpanded={isExpanded}
       label={_("Partitions and file systems")}
       description={_("Structure of the new system, including any additional partition needed for booting,")}

web/src/components/storage/SpacePolicyField.jsx

@@ -26,7 +26,7 @@ import { Skeleton } from "@patternfly/react-core";
 
 import { sprintf } from "sprintf-js";
 import { _, n_ } from "~/i18n";
-import { If, SettingsField } from "~/components/core";
+import { If, Field } from "~/components/core";
 import SpacePolicyDialog from "~/components/storage/SpacePolicyDialog";
 
 /**
@@ -82,10 +82,11 @@ export default function SpacePolicyField({
   }
 
   return (
-    <SettingsField
+    <Field
+      icon="frame_inspect"
       label={_("Find space")}
       value={value}
-      description={ _("Allocating the file systems might need to find free space \
+      description={_("Allocating the file systems might need to find free space \
 in the installation device(s).")}
       onClick={openDialog}
     >
@@ -102,6 +103,6 @@ in the installation device(s).")}
           />
         }
       />
-    </SettingsField>
+    </Field>
   );
 }