Space policy UI #883

jreidinger · 2023-11-21T20:54:11Z

Problem

There is already support in backend for space policy for storage proposal, but UI is missing.

https://trello.com/c/VjNLDTd6/3410-5-storage-ui-to-select-the-settings-to-make-space-except-custom

Solution

Implement UI.

What is missing:

Good representation of the affected devices at the form (including some code refactoring at device-utils.jsx)
Better font layout and style
Improve wording
Support for singular / plural (number of affected devices)

Testing

What is missing:

Manual tests of a full installation (maybe better done after merging, together with other stuff currently at integration phase)
Unit tests

Screenshots

mvidner

Only adjusting the user visible texts, and proposing explanations for them, to be shown optionally, somehow

web/src/components/storage/ProposalSettingsSection.jsx

mvidner · 2023-11-22T12:22:00Z

web/src/components/storage/ProposalSettingsSection.jsx

+            onClick={selectResize}
+          />
+          <ToggleGroupItem
+            text={_("Keep Existing")}


Suggested change

text={_("Keep Existing")}

text={_("Keep Everything")}

Help text

Existing partitions and their data* will not be touched. There needs to be enough unpartitioned space on the target disk already.

*: If there is an EFI boot partition, its contents will be modified to allow booting the system being installed.

am I correct in this?

web/src/components/storage/ProposalSettingsSection.jsx

mvidner · 2023-11-22T12:33:03Z

web/src/components/storage/ProposalSettingsSection.jsx

+  const SpacePolicySettingsButton = () => {
+    return (
+      <Tooltip
+        content={_("Configure the Space Policy")}


Go! For!! Mars!!! (SCNR)

Maybe use Storage Space (Policy) instead of just Space (Policy)? Everywhere?

web/src/components/storage/ProposalSettingsSection.jsx

mvidner · 2023-11-23T14:10:57Z

web/src/components/storage/ProposalSettingsSection.jsx

+    );
+  };
+
+  if (isLoading) return <Skeleton width="25%" />;


This looks like it could be moved to the top of SpacePolicyField, an early return

web/src/components/storage/ProposalSettingsSection.jsx

web/src/components/storage/space-policy-utils.jsx

coveralls · 2023-11-24T14:59:12Z

coverage: 75.62% (+0.05%) from 75.573%
when pulling 34b8d49 on space_policy_ui
into 888aa22 on master.

web/src/components/storage/space-policy-utils.jsx

I think we shouldn't use data-whatever in every place

web/src/client/storage.js

web/src/components/storage/space-policy-utils.jsx

web/src/components/storage/device-utils.jsx

web/src/client/storage.js

To make it possible reusing the same code for rendering either, a list or a selector. There is room for improvements yet, but it should be enough for now to avoid having list marked as "listbox" with elements set as "role=option". This change also proposes a different way to style them, avoiding setting a grid by default for each type of selector since it's not always the case. E.g., the space policy selector has a so simple layout. In any case, this is a subject for changes in the short term since selectors must be reworked ASAP.

dgdavid · 2023-11-29T20:31:52Z

@ancorgs, @mvidner, @joseivanlopez,

I've sent a few commits. I think the space policy dialog looks better now, let me know if there is something you believe should be changed for accepting this PR from the UI perspective.

Remember that in general selectors need more love, but it deserves a separated PR.

BTW, I did a last change but didn't send it because it's something I'd do once #881 and this PR are merged. It's about making selectors more compact by dropping the space between their options. If you are curious, see below the diff and a few screenshots

Click to show/hide the screenshots

Click to show/hide the (NOT SENT) diff

diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss
index 30ac1f73..7218d7ce 100644
--- a/web/src/assets/styles/blocks.scss
+++ b/web/src/assets/styles/blocks.scss
@@ -267,11 +267,21 @@ span.notification-mark[data-variant="sidebar"] {
 ul[data-type="agama/list"] {
   li {
     border: 2px solid var(--color-gray-dark);
-    border-radius: 5px;
     padding: var(--spacer-normal);
     text-align: start;
     background: var(--color-gray-light);
-    margin-block-end: var(--spacer-small);
+
+    &:first-child {
+      border-radius: 5px 5px 0 0;
+    }
+
+    &:last-child {
+      border-radius: 0 0 5px 5px;
+    }
+
+    &:nth-child(n+2) {
+      border-top: 0;
+    }
   }

web/package/cockpit-agama.changes

web/src/client/storage.js

web/src/components/overview/StorageSection.jsx

jreidinger · 2023-11-30T15:36:36Z

web/src/components/storage/ProposalSettingsSection.jsx

+      </Popup>
+    </div>
+  );
+};


NP: btw when I touched it I already have feeling that maybe it should be split to own component file.

I would evaluate it later if this section continues growing. Right now all its *Field components are defined there.

I know that is why I in the end decide to keep it there as it is consistent, but start to be quite big.

They were used for styling some elements that can be styled in a different way. In any case, we still needing something to make these div nodes accessible since now we (sigth people) figure out what they are because we're able to see them well distributed in the UI. But I'm not sure what a mess it can be if something read all these information aloud.

At this time they are not needed for placing the selectors content properly.

Not needed at this moment

jreidinger

LGTM for me ( cannot approved as I am original author )

reduce yupdate requirements due to zram

79f7bac

This comment was marked as outdated.

Sign in to view

mvidner reviewed Nov 22, 2023

View reviewed changes

mvidner reviewed Nov 23, 2023

View reviewed changes

web/src/components/storage/ProposalSettingsSection.jsx Outdated Show resolved Hide resolved

mvidner reviewed Nov 24, 2023

View reviewed changes

web/src/components/storage/ProposalSettingsSection.jsx Outdated Show resolved Hide resolved

web/src/components/storage/space-policy-utils.jsx Show resolved Hide resolved

ancorgs force-pushed the space_policy_ui branch from 1688268 to 8d55d27 Compare November 24, 2023 15:59

This comment was marked as outdated.

Sign in to view

dgdavid reviewed Nov 28, 2023

View reviewed changes

web/src/components/storage/space-policy-utils.jsx Outdated Show resolved Hide resolved

jreidinger and others added 8 commits November 29, 2023 09:35

update documentation

e1df1b7

initial attempt for Space policy

081cfdf

WIP: stylesheet reorganization stoled from another branch

d62f91b

Changes on top of the original SpacePolicy prototype

b4c92e4

[web] Adjust some styles

602016c

I think we shouldn't use data-whatever in every place

SpacePolicy reuses widget for displaying devices

38902df

Prefer internal components over internal functions

de31d49

Comments for translators

14bb25d

ancorgs force-pushed the space_policy_ui branch from 78542a8 to 14bb25d Compare November 29, 2023 09:54

joseivanlopez reviewed Nov 29, 2023

View reviewed changes

web/src/client/storage.js Show resolved Hide resolved

web/src/components/storage/space-policy-utils.jsx Show resolved Hide resolved

web/src/components/storage/device-utils.jsx Show resolved Hide resolved

web/src/client/storage.js Outdated Show resolved Hide resolved

joseivanlopez and others added 5 commits November 29, 2023 12:41

Merge branch 'master' into space_policy_ui

c7bf43e

[web] Adjust styles for PF/Hint and PF/ExpandableSection

acdf481

[web] Structural changes for space policy selector

08049d6

[web] Reduce the use of "stack" utility CSS class

c6ad047

[web] Adapt texts

f786c3c

joseivanlopez and others added 5 commits November 30, 2023 10:26

[web] Always show link to expand affected devices

48ece9f

[web] Add tests

e374fe0

Adapt overview text based on the storage space policy

15b74f7

Grammar fixes

f4bd395

[web] Small fixes

1d5378c

ancorgs marked this pull request as ready for review November 30, 2023 12:22

joseivanlopez added 4 commits November 30, 2023 12:39

[web] Fix test

a57a016

Merge branch 'master' into space_policy_ui

f60f27e

[web] Adapt selectors to new styles

15aab26

[web] Changelog

67944b0

jreidinger commented Nov 30, 2023

View reviewed changes

web/package/cockpit-agama.changes Outdated Show resolved Hide resolved

jreidinger commented Nov 30, 2023

View reviewed changes

web/src/client/storage.js Outdated Show resolved Hide resolved

jreidinger commented Nov 30, 2023

View reviewed changes

web/src/components/overview/StorageSection.jsx Outdated Show resolved Hide resolved

jreidinger commented Nov 30, 2023

View reviewed changes

dgdavid added 4 commits November 30, 2023 15:47

[web] Reduce the use of grid-template-areas

f50a66c

At this time they are not needed for placing the selectors content properly.

[web] Spacing adjustments

253aa39

[web] Drop cursor-pointer CSS utility class

94bb4d4

Not needed at this moment

dgdavid force-pushed the space_policy_ui branch from d0b4ab0 to 94bb4d4 Compare November 30, 2023 16:17

joseivanlopez added 3 commits November 30, 2023 16:29

[web] Improve changelog

03b0a57

[web] Make code more clear

3e306c5

[web] Cover all policy values in switch

34b8d49

dgdavid approved these changes Nov 30, 2023

View reviewed changes

jreidinger commented Nov 30, 2023

View reviewed changes

joseivanlopez merged commit bb1c038 into master Nov 30, 2023
2 checks passed

joseivanlopez deleted the space_policy_ui branch November 30, 2023 16:40

dgdavid mentioned this pull request Nov 30, 2023

[web] Save space when rendering a selector #898

Merged

imobachgs mentioned this pull request Dec 2, 2023

Update to release 6 #904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Space policy UI #883

Space policy UI #883

jreidinger commented Nov 21, 2023 •

edited by ancorgs

Loading

This comment was marked as outdated.

mvidner left a comment

mvidner Nov 22, 2023

mvidner Nov 22, 2023

mvidner Nov 23, 2023

coveralls commented Nov 24, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

dgdavid commented Nov 29, 2023

jreidinger Nov 30, 2023 •

edited

Loading

joseivanlopez Nov 30, 2023

jreidinger Nov 30, 2023

jreidinger left a comment

Space policy UI #883

Space policy UI #883

Conversation

jreidinger commented Nov 21, 2023 • edited by ancorgs Loading

Problem

Solution

Testing

Screenshots

This comment was marked as outdated.

mvidner left a comment

Choose a reason for hiding this comment

mvidner Nov 22, 2023

Choose a reason for hiding this comment

mvidner Nov 22, 2023

Choose a reason for hiding this comment

mvidner Nov 23, 2023

Choose a reason for hiding this comment

coveralls commented Nov 24, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

dgdavid commented Nov 29, 2023

jreidinger Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

joseivanlopez Nov 30, 2023

Choose a reason for hiding this comment

jreidinger Nov 30, 2023

Choose a reason for hiding this comment

jreidinger left a comment

Choose a reason for hiding this comment

jreidinger commented Nov 21, 2023 •

edited by ancorgs

Loading

coveralls commented Nov 24, 2023 •

edited

Loading

jreidinger Nov 30, 2023 •

edited

Loading