From bb7ebe45d968ca5c8252c1a0f87683b98027dcd1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= <dgonzalez@suse.de>
Date: Tue, 16 Apr 2024 11:54:44 +0100
Subject: [PATCH 1/5] web: Use meaningful icons in Storage proposal page

Icons that intend to represent the action's concept instead of how the
UI is going to present the action to the user.
---
 web/src/components/core/Field.jsx             |  5 ++---
 web/src/components/core/Field.test.jsx        | 20 ++++++++-----------
 web/src/components/layout/Icon.jsx            |  4 ++++
 .../components/storage/BootConfigField.jsx    |  6 +++---
 .../components/storage/EncryptionField.jsx    |  7 ++++---
 .../storage/InstallationDeviceField.jsx       |  7 ++++---
 .../components/storage/PartitionsField.jsx    |  1 +
 .../components/storage/SpacePolicyField.jsx   |  9 +++++----
 8 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/web/src/components/core/Field.jsx b/web/src/components/core/Field.jsx
index 74d955733a..bc9533b0c5 100644
--- a/web/src/components/core/Field.jsx
+++ b/web/src/components/core/Field.jsx
@@ -108,13 +108,12 @@ const SwitchField = ({ isChecked = false, highlightContent = false, ...props })
 };
 
 /**
- * @param {Omit<FieldProps, 'icon'> & {isExpanded: boolean}} props
+ * @param {FieldProps & {isExpanded: boolean}} props
  */
 const ExpandableField = ({ isExpanded, ...props }) => {
-  const iconName = isExpanded ? "collapse_all" : "expand_all";
   const className = isExpanded ? "expanded" : "collapsed";
 
-  return <Field {...props} icon={iconName} className={className} />;
+  return <Field {...props} className={className} />;
 };
 
 export default Field;
diff --git a/web/src/components/core/Field.test.jsx b/web/src/components/core/Field.test.jsx
index 2a07467732..af0ba3e3f2 100644
--- a/web/src/components/core/Field.test.jsx
+++ b/web/src/components/core/Field.test.jsx
@@ -111,19 +111,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);
   });
 });
diff --git a/web/src/components/layout/Icon.jsx b/web/src/components/layout/Icon.jsx
index 848d4859cc..813185a42f 100644
--- a/web/src/components/layout/Icon.jsx
+++ b/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,
diff --git a/web/src/components/storage/BootConfigField.jsx b/web/src/components/storage/BootConfigField.jsx
index da2d942b19..b62ccf14fb 100644
--- a/web/src/components/storage/BootConfigField.jsx
+++ b/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={
diff --git a/web/src/components/storage/EncryptionField.jsx b/web/src/components/storage/EncryptionField.jsx
index 2246eed04a..208b2afcd6 100644
--- a/web/src/components/storage/EncryptionField.jsx
+++ b/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>
   );
 }
diff --git a/web/src/components/storage/InstallationDeviceField.jsx b/web/src/components/storage/InstallationDeviceField.jsx
index 2913d24d7b..eb4fb22988 100644
--- a/web/src/components/storage/InstallationDeviceField.jsx
+++ b/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>
   );
 }
diff --git a/web/src/components/storage/PartitionsField.jsx b/web/src/components/storage/PartitionsField.jsx
index 9f883ad8e5..6c8aeafc4d 100644
--- a/web/src/components/storage/PartitionsField.jsx
+++ b/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,")}
diff --git a/web/src/components/storage/SpacePolicyField.jsx b/web/src/components/storage/SpacePolicyField.jsx
index 3208820dcc..7858df0fc7 100644
--- a/web/src/components/storage/SpacePolicyField.jsx
+++ b/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>
   );
 }

From 6076027698e4b6ef81f2720d270c2c2c21fb333d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= <dgonzalez@suse.de>
Date: Tue, 16 Apr 2024 12:07:53 +0100
Subject: [PATCH 2/5] web: Removes no longer useful SettingsField

---
 web/src/components/core/Field.jsx      |  9 +--------
 web/src/components/core/Field.test.jsx | 13 +------------
 web/src/components/core/index.js       |  2 +-
 3 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/web/src/components/core/Field.jsx b/web/src/components/core/Field.jsx
index bc9533b0c5..2d607d6653 100644
--- a/web/src/components/core/Field.jsx
+++ b/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
  */
@@ -117,4 +110,4 @@ const ExpandableField = ({ isExpanded, ...props }) => {
 };
 
 export default Field;
-export { ExpandableField, SettingsField, SwitchField };
+export { ExpandableField, SwitchField };
diff --git a/web/src/components/core/Field.test.jsx b/web/src/components/core/Field.test.jsx
index af0ba3e3f2..6485c230c9 100644
--- a/web/src/components/core/Field.test.jsx
+++ b/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 />);
diff --git a/web/src/components/core/index.js b/web/src/components/core/index.js
index c881137abb..7834183e6f 100644
--- a/web/src/components/core/index.js
+++ b/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";

From f145bd284d6ce09c45d552a7d66f6c2d705ad206 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= <dgonzalez@suse.de>
Date: Tue, 16 Apr 2024 12:15:59 +0100
Subject: [PATCH 3/5] web: Update changelog

---
 web/package/cockpit-agama.changes | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes
index d6817fe86c..6db121f296 100644
--- a/web/package/cockpit-agama.changes
+++ b/web/package/cockpit-agama.changes
@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Tue Apr 16 11:15:13 UTC 2024 - David Diaz <dgonzalez@suse.com>
+
+- Use better icons in storage proposal page
+  (gh#openSUSE/agama#1152).
+
 -------------------------------------------------------------------
 Mon Apr 15 10:52:14 UTC 2024 - David Diaz <dgonzalez@suse.com>
 

From 196a622966c435fa39918aede5f893f09fdd208f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= <dgonzalez@suse.de>
Date: Tue, 16 Apr 2024 12:21:09 +0100
Subject: [PATCH 4/5] web: Use status icon in the ExpandableField label

To somehow keeps the representation that the field is expanded or not.
This should be considered a workaround and must be changed to something
more expressive, meaningful, and usable: a "Basic View | Advanced View"
control.
---
 web/src/components/core/Field.jsx | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/web/src/components/core/Field.jsx b/web/src/components/core/Field.jsx
index 2d607d6653..93c2f26a44 100644
--- a/web/src/components/core/Field.jsx
+++ b/web/src/components/core/Field.jsx
@@ -103,10 +103,12 @@ const SwitchField = ({ isChecked = false, highlightContent = false, ...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 iconizedLabel = <>{label} <Icon name={iconName} size="xs" /></>;
 
-  return <Field {...props} className={className} />;
+  return <Field {...props} label={iconizedLabel} className={className} />;
 };
 
 export default Field;

From 17f45a03ffede8bf4479cdf660a9e71aaba49b9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= <dgonzalez@suse.de>
Date: Tue, 16 Apr 2024 12:27:17 +0100
Subject: [PATCH 5/5] web: Please CSpell

---
 web/src/components/core/Field.jsx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/web/src/components/core/Field.jsx b/web/src/components/core/Field.jsx
index 93c2f26a44..7e63dc49ba 100644
--- a/web/src/components/core/Field.jsx
+++ b/web/src/components/core/Field.jsx
@@ -106,9 +106,9 @@ const SwitchField = ({ isChecked = false, highlightContent = false, ...props })
 const ExpandableField = ({ label, isExpanded, ...props }) => {
   const iconName = isExpanded ? "collapse_all" : "expand_all";
   const className = isExpanded ? "expanded" : "collapsed";
-  const iconizedLabel = <>{label} <Icon name={iconName} size="xs" /></>;
+  const labelWithIcon = <>{label} <Icon name={iconName} size="xs" /></>;
 
-  return <Field {...props} label={iconizedLabel} className={className} />;
+  return <Field {...props} label={labelWithIcon} className={className} />;
 };
 
 export default Field;