Find space section #1028
Merged
Find space section #1028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joseivanlopez
force-pushed
the
space-policy-ui
branch
from
185556c to
bebdfe5
Compare
- Moreover, the list of actions is always recovered from Y2Storage instead of remembering the actions passed in the settings to calculate a proposal.
joseivanlopez
force-pushed
the
space-policy-ui
branch
from
bebdfe5 to
95d9a92
Compare
joseivanlopez
force-pushed
the
space-policy-ui
branch
from
95d9a92 to
0028356
Compare
- Read recoverable size from D-Bus. - Create partition objects
joseivanlopez
force-pushed
the
space-policy-ui
branch
from
dc807a0 to
421ff9d
Compare
- Wording, use FormSelect, adapt loading state
No longer needed with the new approach in which the space policy selection has its own section.
dgdavid
reviewed
Feb 15, 2024
dgdavid
reviewed
Feb 15, 2024
According to @ancorgs's suggestion.
Which helps rendering a set of options with a tile look&feel. Not using PatternFly/Tile instead because it looks like it's going to be deprecated [1] and the PatternFly/Card#isSelectable component is a bit complex for this first use case. [1] patternfly/patternfly-react#8542
dgdavid
reviewed
Feb 16, 2024
| <div>{device.name}</div> | ||
| <If | ||
| condition={isDrive(device)} | ||
| then={<div className="fs-small">{`${device.vendor} ${device.model}`}</div>} |
There was a problem hiding this comment.
Note for another iteration: use something different than a smaller size.
| else | ||
| text = sprintf(_("%s recoverable"), deviceSize(device.recoverableSize)); | ||
|
|
||
| return <div className="fs-small">{text}</div>; |
There was a problem hiding this comment.
Note for another iteration: try something more than simply smaller text. Also check how these "less important" information should be structured in terms of a11y.
Comment on lines
213
to
222
| <FormSelect | ||
| value={action} | ||
| isDisabled={isDisabled} | ||
| onChange={changeAction} | ||
| aria-label="Space action selector" | ||
| > | ||
| <FormSelectOption value="force_delete" label={_("Delete")} /> | ||
| <FormSelectOption value="resize" label={_("Allow resize")} /> | ||
| <FormSelectOption value="keep" label={_("Do not modify")} /> | ||
| </FormSelect> |
There was a problem hiding this comment.
Note for another iteration, and related with a discussion I had with @ancorgs yesterday: stop using form controls for actions that are going to be executed/applied right away on user interaction. This is a bit more difficult than exchanging a checkbox by a switch, but for sure we will find something.
Comment on lines
+301
to
+367
| const SpaceActionsTable = ({ settings, onChange = noop }) => { | ||
| const [expandedDevices, setExpandedDevices] = useLocalStorage("storage-space-actions-expanded", []); | ||
| const [autoExpanded, setAutoExpanded] = useLocalStorage("storage-space-actions-auto-expanded", false); | ||
|
|
||
| useEffect(() => { | ||
| const policy = settings.spacePolicy; | ||
| const devices = settings.installationDevices.map(d => d.name); | ||
| let currentExpanded = devices.filter(d => expandedDevices.includes(d)); | ||
|
|
||
| if (policy === "custom" && !autoExpanded) { | ||
| currentExpanded = [...devices]; | ||
| setAutoExpanded(true); | ||
| } else if (policy !== "custom" && autoExpanded) { | ||
| setAutoExpanded(false); | ||
| } | ||
|
|
||
| if (currentExpanded.sort().toString() !== expandedDevices.sort().toString()) { | ||
| setExpandedDevices(currentExpanded); | ||
| } | ||
| }, [autoExpanded, expandedDevices, setAutoExpanded, setExpandedDevices, settings]); | ||
|
|
||
| const renderRows = () => { | ||
| const rows = []; | ||
|
|
||
| settings.installationDevices?.forEach((device, index) => { | ||
| const isExpanded = expandedDevices.includes(device.name); | ||
| const children = device.partitionTable?.partitions; | ||
|
|
||
| const onCollapse = () => { | ||
| const otherExpandedDevices = expandedDevices.filter(name => name !== device.name); | ||
| const expanded = isExpanded ? otherExpandedDevices : [...otherExpandedDevices, device.name]; | ||
| setExpandedDevices(expanded); | ||
| }; | ||
|
|
||
| rows.push( | ||
| <DeviceRow | ||
| key={device.name} | ||
| device={device} | ||
| settings={settings} | ||
| rowIndex={rows.length} | ||
| setSize={children?.length || 0} | ||
| posInSet={index} | ||
| isExpanded={isExpanded} | ||
| onCollapse={onCollapse} | ||
| onChange={onChange} | ||
| /> | ||
| ); | ||
|
|
||
| children?.forEach((child, index) => { | ||
| rows.push( | ||
| <DeviceRow | ||
| key={child.name} | ||
| device={child} | ||
| settings={settings} | ||
| rowIndex={rows.length} | ||
| level={2} | ||
| posInSet={index} | ||
| isHidden={!isExpanded} | ||
| onCollapse={onCollapse} | ||
| onChange={onChange} | ||
| /> | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| return rows; | ||
| }; |
There was a problem hiding this comment.
Note for another iteration: as we were talking, most probably this deserves a core/TreeTable wrapper or so. Specially given the fact we are adding an extra feature to the component provided by the library (the state saved into localStorage).
Comment on lines
444
to
446
| <p> | ||
| {_("Indicate how to make free space in the selected disks for allocating the file systems:")} | ||
| </p> |
There was a problem hiding this comment.
Note for myself: I start feeling that core/Section should re-introduce the description or helpText prop for this kind of inline help. We will see.
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
|
|
||
| const filesystem = device.filesystem; | ||
| if (filesystem?.isEFI) return _("EFI"); | ||
| if (filesystem) return sprintf(_("%s file system"), filesystem?.type); |
There was a problem hiding this comment.
I am not sure if that "file system" suffix won't be confusing. E.g. "Btrfs file system" will mean "B tree file system file system" or ntfs or even nfs will be strange.
There was a problem hiding this comment.
I think it is ok "Btrfs file system" or "ntfs file system". At the end, we use "Btrfs", "Xfs", etc as a name.
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
jreidinger
reviewed
Feb 19, 2024
| then={ | ||
| <DeviceActionColumn | ||
| device={device} | ||
| action={spaceAction?.action || "keep"} |
There was a problem hiding this comment.
should we decide default action at this UI level in table? Maybe we should be strict and return action always from backend code?
There was a problem hiding this comment.
Well, if the backend does not specify an action then there is no action (it will do nothing with that device). Using "keep" for representing "no action" is something in the side of the UI code.
jreidinger
reviewed
Feb 19, 2024
Sometimes could be needed to reset the window.localStorage cache between test examples, as it was the case for recently added tests for the space policy section. Thus, instead of having such utility hidden in and repeated in needed tests, it has been moved to a test-utils.js file where, with a bit of luck it will be more discoverable for others. Additionally, that new helpers allows to set an initial cache too by passing it an object with a collection of items compatible with Web Storage API setItem method.
dgdavid
force-pushed
the
space-policy-ui
branch
from
03309f6 to
8e247ff
Compare
joseivanlopez
force-pushed
the
space-policy-ui
branch
from
0c364dd to
411664b
Compare
|
@jreidinger we are going to merge because almost all your suggestions were implemented. Anyway, don't hesitate to add more comments if you still miss something. Thanks! |
Merged
imobachgs
added a commit
that referenced
this pull request
May 17, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The page for configuring storage allows to select the policy for making free space: delete, resize or keep partitions. Those policies affect to all the partitions of the disks used for the installation. But there is no way to select the custom policy in which the user has to choose what to do with each partition (delete, keep or allow to resize).
https://trello.com/c/9AhV6aOb/3406-13-agama-ui-for-custom-making-space
Solution
Add a new section to configure the space policy and the actions for the custom policy. The UI solution is based on the mockups for the new storage UI, see https://github.com/openSUSE/agama/blob/master/doc/storage_ui.md.
Testing
Screenshots