web: Reuse devices #1165

joseivanlopez · 2024-04-24T08:29:21Z

Implement feature for reusing an existing device (i.e., allow formatting a block device or mounting a file system). Main changes:

Redesign the dialog for changing location:
- Shows a table with all the devices that can be selected as location for the volumes.
- Allows selecting how to allocate the file system at selected device (create partition, create dedicated LVM, format, mount).
Add a menu option in the table of volumes for resetting the location (i.e., to use the installation device instead of a custom location).

Note: We have detected some problems with the UI for changing location implemented in this PR:

The table is scrollable. If there are too many devices, then there is a chance of overlooking some devices if you don't realize about the scroll.
Having a table and group of radios at the end looks quite overwhelming. The very same information could be provided in a simpler way.
Ideally, this change location option should be integrated in the form of the volume.

Check some ideas in the thread of this PR and at #1176.

Toggle screenshots

In a temporary wrong placement. Incoming commit will fix it by refactoring the code for building the volume actions.

To make it a bit simpler and keep actions in the desired order.

Although these components are just convenient internal components of VolumeRow, they can be defined outside of it for helping to reduce the components redefinition each time the parent component gets rendered.

- Unify columns. - Activate type checking. - Refactoring code.

coveralls · 2024-04-30T11:42:01Z

coverage: 75.032% (-0.01%) from 75.046%
when pulling ca7e244 on joseivanlopez:reuse-device
into a554ab0 on openSUSE:master.

- Volumes were using the human string of the file system type, but the devices were using a different represetation for the file system type.

- Documentation - Types

To be precise, changes try to force the content to fit in the available space by adding scroll to the devices table if needed in an attempt to keep allocation options always visible. This is temporary and should be changed/improved.

To create more difference between them and the radio descriptions. As previous changes, this aims to be a temporary solution and should be re-evaluated.

* Hide the expand button * Indent children devices

web/src/assets/styles/composition.scss

Relying in the Lea Verou technique described at https://lea.verou.me/blog/2012/04/background-attachment-local. It makes the scrollable table a bit more evident, but still feeling as a workaround.

dgdavid · 2024-05-02T17:02:54Z

@ancorgs, @joseivanlopez

I've sent the latest changes we were talking about this afternoon, please check e2ae65d and screenshots below.

Table not scrolled	Table scrolled

Although making it work was a bit more difficult than I though, I think it is enough for mitigating the inherited problems from an scrollable HTML node in the middle of the UI because the browsers hiding the scroll bars.

However, I still thinking we have chosen the wrong layout here. If I'm able to find time, I will try to come up with an example with a different one to see if I'm right and, if so, I'm able to convince you about it.

In any case, let's considered this scrollbox thingy implementation as temporary workaround that must be improved if finally it stays.

dgdavid · 2024-05-02T17:05:54Z

Although making it work was a bit more difficult than I though

BTW, while scrolling you will see how these chips in the "Usage" column overlaps the scroll shadow. I'm sorry but at this time I do not know how to fix it. I was playing with the z-index property without success. I'll try it again early in the morning, with a fresh mind :)

dgdavid · 2024-05-02T18:45:28Z

I will try to come up with an example

Here you have a rough test. Of course, need refinements (as use just one column in very small devices), but I hope it can be used for evaluation.

Click to show/hide screenshots

Kind of medium resolution

Bigger resolution

Changes can be found at ca0ad81

I might be completely wrong but at this time I'm convinced that it provides a more guided, clear, and less prone to surprises experience.

dgdavid · 2024-05-02T21:24:07Z

I will try to come up with an example

Here you have a rough test. Of course, need refinements (as use just one column in very small devices), but I hope it can be used for evaluation.

...

Let's continue feeding thoughts.

How could fit the use of a select instead of a collection of radios?

Click to show/hide screenshots

Draft code available at dd3c5fe

PS: note that selector already comes with keyboard support.

web/src/components/storage/VolumeLocationSelectorTable.jsx

dgdavid · 2024-05-02T22:22:51Z

Let's continue feeding thoughts.

How could fit the use of a select instead of a collection of radios?
...

And now stop using the table for selecting devices and goes for a select too...

Click to show/hide screenshots

But with better format for the disk content information (it was just stolen from the table for the test). See 09ad6b4

I'm going to stop here, but please take your time for thinking about this before outright rejecting any of the proposals (for future iterations, sure). Specially, try to imagine the last one with better styles and horizontal alignment

A good label

| Format ....  ⌄ |           | /dev/vdc1....  ⌄ |

Apart from a good, simple, keyboard navigation... it could be easily integrated in the VolumeDialog as yet another field.

Also, the device selector could use the https://www.patternfly.org/components/menus/select#typeahead feature.

Disclaimer: I'm not saying to go for it. Just to, at least, arrange time for evaluating it. Maybe we don't need tables everywhere.

ancorgs · 2024-05-02T22:43:13Z

Talking about ideas... #1176

ancorgs · 2024-05-03T10:30:46Z

Some notes about review.

We agreed we want to merge the UI implemented in this pull request... just as a first approach with close expiration date. We need to merge it in order to proceed with other important tasks.

The idea is to substitute this with an UI based on drop-downs (as sketched in a previous comment by @dgdavid). I would suggest to start with that next week.

I tested this pull request and works as expected. So from a functional POV, it LGTM.

But code-wise I would prefer @dgdavid to review it (taking into account some parts would be short-lived, as explained above).

dgdavid

LGTM, what a big and nice job. Thank you.

Please, do not take into consideration my NP comments. Think on them as just reminders for myself to discuss a few things in a near future.

From my side, you can go ahead with the merge 💪

web/src/client/storage.js

dgdavid · 2024-05-03T10:37:38Z

web/src/client/storage.js

+
+    /** @type {(device: StorageDevice) => boolean} */
+    const isAvailable = (device) => {
+      const isChildren = (device, parentDevice) => {


NP: If I'm not wrong, we already have a isChildren function somewhere. Anyway, this kind of functions looks like a potential util that should live elsewhere.

Yes, there is a #deviceChildren function to generate the list of children. In this case the function checks if it is a children of a device. Anyway, I think we need to improve this part, see https://github.com/openSUSE/agama/pull/1165/files#diff-8cfada04978222410b4703fc1ca52a1ff139c6497329b3ca2e4e176ba5d14d00R26.

dgdavid · 2024-05-03T10:44:03Z

web/src/components/core/ExpandableSelector.jsx

- * @param {string[]} [props.initialExpandedKeys=[]] - Ids of initially expanded items.
- * @param {(selection: Array<object>) => void} [props.onSelectionChange=noop] - Callback to be triggered when selection changes.
- * @param {object} [props.tableProps] - Props for {@link https://www.patternfly.org/components/table/#table PF/Table}.
+ * @typedef {object} ExpandableSelectorBaseProps


NP: Just a note not related with this PR. We are following this pattern of defining ComponentBaseProp and for now it is ok. But I wonder if, since it is something internal, we could call them just BaseProp instead. I see two problems, however:

When a single file can end up with more than one BaseProp. Weird.

@typedef auto exports types defined at top level, https://alexharri.com/blog/jsdoc-as-an-alternative-typescript-syntax#exporting-types

dgdavid · 2024-05-03T10:57:10Z

web/src/components/storage/VolumeLocationSelectorTable.jsx

+import { ExpandableSelector } from "~/components/core";
+
+/**
+ * @typedef {import("../core/ExpandableSelector").ExpandableSelectorColumn} ExpandableSelectorColumn


NP: Did ~/components/core/ExpandableSelector not work? Or maybe did you though it is too long? By the way, maybe ExpandableSelectorColumn can be names just Column in core/ExpandableSelector.jsx You have to rename it anyway when importing it.

* @typedef {import("~/components/core/ExpandableSelector").Column} ExpandableSelectorColumn

Again, not related with this PR but this kind of things makes me tend to think that we are actually running away of TypeScript with no strong reason.

Good catch! The path was automatically added by my editor :).

Prepare for releasing Agama 8. It includes the following pull requests: * #884 * #886 * #914 * #918 * #956 * #957 * #958 * #959 * #960 * #961 * #962 * #963 * #964 * #965 * #966 * #969 * #970 * #976 * #977 * #978 * #979 * #980 * #981 * #983 * #984 * #985 * #986 * #988 * #991 * #992 * #995 * #996 * #997 * #999 * #1003 * #1004 * #1006 * #1007 * #1008 * #1009 * #1010 * #1011 * #1012 * #1014 * #1015 * #1016 * #1017 * #1020 * #1022 * #1023 * #1024 * #1025 * #1027 * #1028 * #1029 * #1030 * #1031 * #1032 * #1033 * #1034 * #1035 * #1036 * #1038 * #1039 * #1041 * #1042 * #1043 * #1045 * #1046 * #1047 * #1048 * #1052 * #1054 * #1056 * #1057 * #1060 * #1061 * #1062 * #1063 * #1064 * #1066 * #1067 * #1068 * #1069 * #1071 * #1072 * #1073 * #1074 * #1075 * #1079 * #1080 * #1081 * #1082 * #1085 * #1086 * #1087 * #1088 * #1089 * #1090 * #1091 * #1092 * #1093 * #1094 * #1095 * #1096 * #1097 * #1098 * #1099 * #1100 * #1102 * #1103 * #1104 * #1105 * #1106 * #1109 * #1110 * #1111 * #1112 * #1114 * #1116 * #1117 * #1118 * #1119 * #1120 * #1121 * #1122 * #1123 * #1125 * #1126 * #1127 * #1128 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1135 * #1136 * #1138 * #1139 * #1140 * #1141 * #1142 * #1143 * #1144 * #1145 * #1146 * #1147 * #1148 * #1149 * #1151 * #1152 * #1153 * #1154 * #1155 * #1156 * #1157 * #1158 * #1160 * #1161 * #1162 * #1163 * #1164 * #1165 * #1166 * #1167 * #1168 * #1169 * #1170 * #1171 * #1172 * #1173 * #1174 * #1175 * #1177 * #1178 * #1180 * #1181 * #1182 * #1183 * #1184 * #1185 * #1187 * #1188 * #1189 * #1190 * #1191 * #1192 * #1193 * #1194 * #1195 * #1196 * #1198 * #1199 * #1200 * #1201 * #1203 * #1204 * #1205 * #1206 * #1207 * #1208 * #1209 * #1210 * #1211 * #1212 * #1213 * #1214 * #1215 * #1216 * #1217 * #1219 * #1220 * #1221 * #1222 * #1223 * #1224 * #1225 * #1226 * #1227 * #1229

joseivanlopez force-pushed the reuse-device branch 2 times, most recently from f19083c to c080dd8 Compare April 26, 2024 14:02

dgdavid added 4 commits April 26, 2024 15:03

web: Put "Delete" volume option at the end

4c082bf

web: Add the "Reset location" volume action

1a107bd

In a temporary wrong placement. Incoming commit will fix it by refactoring the code for building the volume actions.

web: Refactor VolumeActions internal component

0a18ab9

To make it a bit simpler and keep actions in the desired order.

web: Extract few components from VolumeRow

dd8ce25

Although these components are just convenient internal components of VolumeRow, they can be defined outside of it for helping to reduce the components redefinition each time the parent component gets rendered.

joseivanlopez force-pushed the reuse-device branch from c080dd8 to 5420b85 Compare April 26, 2024 14:06

web: Improvements in tables

dc24960

- Unify columns. - Activate type checking. - Refactoring code.

joseivanlopez force-pushed the reuse-device branch from 5420b85 to dc24960 Compare April 26, 2024 14:15

dgdavid and others added 2 commits April 26, 2024 15:38

web: Fix a bunch of typos

d88d1f0

web: WIP first version of the new location dialog

7fb343b

joseivanlopez force-pushed the reuse-device branch from aa8e87c to 7fb343b Compare April 30, 2024 11:37

service: Fix bug converting settings from Y2Storage

66b9b00

joseivanlopez and others added 10 commits April 30, 2024 13:29

web: Improve wording

4bcef6b

service: Always export the file system human string

7a4f457

- Volumes were using the human string of the file system type, but the devices were using a different represetation for the file system type.

web: Remove unnecessary conversion for file system type

dc6c80f

web: Do not consider reused devices as installation devices

07f850d

wer: Add alert when editing volume

bded1d0

web: Several improvements

a9b8d0d

- Documentation - Types

web: Force two rows layout in VolumeLocationDialog

3d479a8

To be precise, changes try to force the content to fit in the available space by adding scroll to the devices table if needed in an attempt to keep allocation options always visible. This is temporary and should be changed/improved.

web: Increase a bit the size of radio labels

78a57ae

To create more difference between them and the radio descriptions. As previous changes, this aims to be a temporary solution and should be re-evaluated.

web: Add spaces between VolumeLocationDialog elements

d458000

web: CSS tweaks for VolumeLocationDialog table

9bfedf7

* Hide the expand button * Indent children devices

dgdavid reviewed May 2, 2024

View reviewed changes

web/src/assets/styles/composition.scss Outdated Show resolved Hide resolved

joseivanlopez and others added 5 commits May 2, 2024 13:50

service: Do not set device if reusing a device

98a46ac

web: Small fixes in styles

b2d3512

web: Small visual fix at DeviceSelectorTable

fd5d7be

web: Add tests for VolumeLocationDialog

82f7be5

web: Small fixes

b397231

joseivanlopez and others added 4 commits May 2, 2024 16:26

web: Fix some tests

1460a2d

Merge branch 'master' into reuse-device

a848039

web: Fix and adapt some tests

e77b83a

web: Make scrollable table more discoverable

e2ae65d

Relying in the Lea Verou technique described at https://lea.verou.me/blog/2012/04/background-attachment-local. It makes the scrollable table a bit more evident, but still feeling as a workaround.

web: Please CSpell

546f82c

dgdavid reviewed May 2, 2024

View reviewed changes

web/src/components/storage/VolumeLocationSelectorTable.jsx Show resolved Hide resolved

dgdavid reviewed May 2, 2024

View reviewed changes

web/src/components/storage/VolumeLocationSelectorTable.jsx Show resolved Hide resolved

joseivanlopez added 2 commits May 3, 2024 07:42

web: Add and adapt tests

068e7b3

web: Minor fixes

f4ce92b

joseivanlopez marked this pull request as ready for review May 3, 2024 08:47

web: Changelog

2dddefa

dgdavid approved these changes May 3, 2024

View reviewed changes

web: Improvements from review

ca7e244

joseivanlopez merged commit aa89adb into agama-project:master May 3, 2024
4 checks passed

imobachgs mentioned this pull request May 17, 2024

Update to release 8 #1232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: Reuse devices #1165

web: Reuse devices #1165

joseivanlopez commented Apr 24, 2024 •

edited

Loading

coveralls commented Apr 30, 2024 •

edited

Loading

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024 •

edited

Loading

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024

ancorgs commented May 2, 2024

ancorgs commented May 3, 2024

dgdavid left a comment

dgdavid May 3, 2024

joseivanlopez May 3, 2024

dgdavid May 3, 2024

dgdavid May 3, 2024

joseivanlopez May 3, 2024

web: Reuse devices #1165

web: Reuse devices #1165

Conversation

joseivanlopez commented Apr 24, 2024 • edited Loading

coveralls commented Apr 30, 2024 • edited Loading

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024 • edited Loading

dgdavid commented May 2, 2024

dgdavid commented May 2, 2024

ancorgs commented May 2, 2024

ancorgs commented May 3, 2024

dgdavid left a comment

Choose a reason for hiding this comment

dgdavid May 3, 2024

Choose a reason for hiding this comment

joseivanlopez May 3, 2024

Choose a reason for hiding this comment

dgdavid May 3, 2024

Choose a reason for hiding this comment

dgdavid May 3, 2024

Choose a reason for hiding this comment

joseivanlopez May 3, 2024

Choose a reason for hiding this comment

joseivanlopez commented Apr 24, 2024 •

edited

Loading

coveralls commented Apr 30, 2024 •

edited

Loading

dgdavid commented May 2, 2024 •

edited

Loading