Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

web: add Result section to Storage page #1088

Merged
merged 39 commits into from
Mar 18, 2024
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Mar 11, 2024

Replace the Actions section in the storage proposal for a Result one which presents the proposal result in a more appealing way displaying how the storage would look after installation instead of just the list of actions. Among others bits, it hints the user about things like file-systems are going to be created, resized actions and a subtle emphasis in deletions, if any.

The full, plain list of actions is now available in a modal dialog linked right before the new introduced table.

Some screenshots

Proposal with errors Proposal without deletion actions
Screenshot from 2024-03-14 09-50-07 Screenshot from 2024-03-14 09-46-47
Proposal with deletion actions List of actions dialog
Screenshot from 2024-03-14 09-51-36 Screenshot from 2024-03-14 09-51-41

⚠️ The screenshots above are just a bit updated: since the collapse/expand feature is not yet implemented for the Result table, the arrow toggles has been hiding with a nasty CSS hack. The plan could be either, to add the collapsible feature to the table or to see how to tell PF/TreeTable to render the structure without the collapsible feature... if that's is possible.


Replaces #1078

@dgdavid dgdavid changed the title [WIP]: web: Add Result section to Storage page. [WIP]: web: Add Result section to Storage page Mar 11, 2024
@dgdavid dgdavid force-pushed the storage-result-section branch 2 times, most recently from 4a7754a to 72b7a57 Compare March 12, 2024 15:36
@coveralls
Copy link

coveralls commented Mar 13, 2024

Coverage Status

coverage: 75.0% (+0.3%) from 74.711%
when pulling f7cb0e4 on storage-result-section
into 0574914 on storage-ui.

*
* @returns {StorageDevice[]}
*/
deletedDevices() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that method could receive a flag for excluding subvolumes. Use case when rendering the amount of planned delete actions. What is more, I wonder why we have to filter the subvolume actions there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that method could receive a flag for excluding subvolumes. Use case when rendering the amount of planned delete actions.

This method cannot be used for counting the amount of delete actions. It returns the deleted devices (e.g., partitions, VGs, MD RAIDs, etc). The number of delete actions can be more than the number of deleted devices.

To know the number of delete actions:

devicesManager.actions.filter(a => a.delete).length

And if you want to exclude the subvolume actions, then:

devicesManager.actions.filter(a => a.delete && !a.subvol).length

What is more, I wonder why we have to filter the subvolume actions there.

I don't get this. Where are the subvolume actions filtered out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is more, I wonder why we have to filter the subvolume actions there.

I don't get this. Where are the subvolume actions filtered out?

Nevermind. I meant that we have a method for filtering the subvolumes from actions in the ProposalResultSection.jsx at line ~232. But we have a "DevicesManager" object that provides a "deleteDevices" method and, in my opinion, could provide a "deleteSystems" too. Although I konw, it is a "DeviceManager" not a "SystemsManager".

@dgdavid dgdavid changed the title [WIP]: web: Add Result section to Storage page Add Result section to Storage page Mar 14, 2024
@dgdavid dgdavid marked this pull request as ready for review March 14, 2024 09:36
@dgdavid dgdavid force-pushed the storage-result-section branch from 1ef1511 to 0bd8ded Compare March 14, 2024 13:20
@dgdavid dgdavid changed the title Add Result section to Storage page web: add Result section to Storage page Mar 14, 2024
ancorgs added a commit that referenced this pull request Mar 15, 2024
## Problem

A new table is going to be added to present the storage result, see
#1088. Having both the table for
configuring the space policy and the table for the result at the same
page is too much.

## Solution

Move the space policy configuration to a popup.

## Screenshots

<details>
<summary>Toggle</summary>


![one](https://github.com/openSUSE/agama/assets/3638289/39733341-3ffa-4ef5-946c-5f9c965fc343)


![another](https://github.com/openSUSE/agama/assets/3638289/daa6fd77-d5a5-47e5-b61c-9d6cee2f982e)

</summary>
</details>
const renderNewLabel = (item) => {
if (!item.sid) return;

// FIXME New PVs over a disk is not detected as new.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need a Trello card collecting all the shortcuts taken and the corner-cases found on this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking it twice, I don't think that can ever happen. The proposal only create PVs on partitions. The GuidedProposal is partition-driven. It can reuse a given VG, LV or partition, but it will never turn a block device into a PV without partitioning it.

@joseivanlopez joseivanlopez changed the base branch from storage-ui to master March 15, 2024 12:06
@joseivanlopez joseivanlopez changed the base branch from master to storage-ui March 15, 2024 12:07
dgdavid and others added 6 commits March 15, 2024 12:42
Instead of using an intrusive warning since it is too early to do so. A
subtle reminder is enough in the storage proposal page, postponing the
warning to the moment right before start the installation if finally it
is needed.
And improve a bit the warning. Now the actions summary is placed at the
top of the section, near to the description which already hints about
the amount actions when there is a valid proposal.
@joseivanlopez joseivanlopez force-pushed the storage-result-section branch from 6501947 to ed6f01c Compare March 15, 2024 12:44
@dgdavid
Copy link
Contributor Author

dgdavid commented Mar 15, 2024

@ancorgs, @joseivanlopez

I've sent commit f13fd87 to fix the Device column alignment. The whole thing about the CSS overrides deserves a review in later iterations, but now it looks better.

Before After
Screenshot from 2024-03-15 17-02-20 Screenshot from 2024-03-15 17-02-08

@dgdavid dgdavid force-pushed the storage-result-section branch from e51a01a to 34342ba Compare March 15, 2024 17:33
@dgdavid
Copy link
Contributor Author

dgdavid commented Mar 15, 2024

I've "fixed" the problem with not needed "cuts" in column headers. See screenshot below forcing to show the Resize label to check that Mount point column does not get affect.

As said above, all of this needs a refinement but for now I prefer merging these styles as they are and apply all the improvement all together in a specific commit.

Screenshot from 2024-03-15 17-33-06

@dgdavid dgdavid force-pushed the storage-result-section branch from 34342ba to 195e962 Compare March 16, 2024 22:17
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I would just add another comment for translators.

And also delete a leftover interpolation.
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dgdavid dgdavid merged commit 32c4639 into storage-ui Mar 18, 2024
5 checks passed
@dgdavid dgdavid deleted the storage-result-section branch March 18, 2024 09:18
dgdavid added a commit that referenced this pull request Mar 19, 2024
As part of the Storage UI changes described at
https://github.com/openSUSE/agama/blob/master/doc/storage_ui.md
document, this PR aims to merge into master the changes already
available in the `storage_ui` feature branch, which has been already
tested and validated. Namely,

* #1071

   Added the following information to D-Bus:

    * The list of actions includes the SID of the affected device.
    * The partition table exports the unused slots.
* The LVM devices (volume groups, physical volumes and logical volumes)
are exported.
* The block devices includes their start block and also indicates
whether the device is encrypted.
    * The staging devices are exported.
    
* #1079 

Adapt the storage client to the changes in the D-Bus API and adapt
`ProposalPage` component to read the information about the devices if
needed.


* #1082

Added new D-Bus interfaces `Device` , `Partition`, `LVM.LogicalVolume`
and adapt `Filesystem` interface.

  It requires yast/yast-storage-ng#1373.

* #1088

Replaced the `Planned Actions` section in the storage proposal for a
`Result` one which presents how the storage would look after
installation instead of just the list of actions.

* #1090

  Moved the space policy configuration to a popup.


* #1098

Replace a `Resize` by `Before` label as it was suggested during the
presentation of the UI in a review meeting.
@imobachgs imobachgs mentioned this pull request May 17, 2024
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants