Reorganize and redefine some storage menus #2241

ancorgs · 2025-04-02T14:26:35Z

Problem

We recently added support for LVM to the storage section of the web interface. But we lacked time to polish some details like:

It's too easy to end up with a configuration without drives as a result of deleting a volume group.
Moving the initial partition-based setup to an LVM is not as obvious as we wanted it to be.

Additionally, the current interface is problematic when the name of a disk is too long (eg. multipath devices) or when there are too many disks to choose from.

Solution

This reorganizes the menus a bit to improve the overall situation, although it probably does not fully fix any of the problems in a definitive way.

First of all, it identifies the disks using their basenames instead of full device names. See more about device names below. In addition, this truncates very long base names to 17 characters at several places (although it uses a wrong approach). These changes do not fully fix the existing problems with long names but mitigates them to some extend.
This also redefines the action to delete an LVM specification. If possible, the system now will move the LVs as partitions to the underlying disk. If that is not possible (multiple underlying disks) or there are no LVs defined, then some sensible deletion of the underlying drives comes into place. That will never lead to the deletion of drives that are used for other purposes or to a setup with no drives.
The menu to change the target of a drive was extracted to a new component DriveDeviceMenu and its options were reorganized:
- Now the option to change the disk is isolated into its own submenu. That's more clear and should also help to handle scenarios with many disks in the future.
- An option to create an LVM in the disk was added. That should ease the creation of LVM-based setups without the user visiting the "more devices" menu.

Implementation details

The new DriveDeviceMenu and the already existing ConfigureDeviceMenu components are now based on new core/MenuButton component. More menus could be adapted in the future.

Pending

Maybe: Add new unit tests

About device names

Most disks have a device name like /dev/sda or /dev/nvme0n1. But if multipath is activated with its default configuration, every disk will get a name that is based on a world-wide unique WWID. See some examples of disks and partition names taken from a real system in which multipath was activated:

/dev/mapper/360050768028383d7f000000000000195
/dev/mapper/360050768028383d7f000000000000196
/dev/mapper/360050768028383d7f000000000000197
/dev/mapper/360050768028383d7f000000000000198
/dev/mapper/360050768028383d7f000000000000199
/dev/mapper/360050768028383d7f00000000000019a
/dev/mapper/360050768028383d7f000000000000195-part1
/dev/mapper/360050768028383d7f000000000000195-part2
/dev/mapper/360050768028383d7f000000000000195-part3
/dev/mapper/360050768028383d7f000000000000195-part4
/dev/mapper/360050768028383d7f000000000000196-part1
/dev/mapper/360050768028383d7f000000000000196-part2

- Unit tests still pending

- The menu sets the aria-current property to the expanded item. - Useful for unit tests to detect whether a submenu is expanded. - Note that #toBeVisible cannot be used because Jsdom does not report correct styles, see jsdom/jsdom#2986.

- Avoid repeating the size of the drive name.

ancorgs · 2025-04-15T11:43:26Z

The part about avoiding the hardcoded "20" for the truncates is just partially done. At the time of writing this comment there are another 4 occurrences that were overlooked (at several components like DriveDeviceMenu, LvmPage, PartitionPage or VolumeGroupEditor.

I will try to commit an alternative solution.

web/src/components/storage/ConfigureDeviceMenu.tsx

This kind of reverts 0f52cfe

ancorgs · 2025-04-15T13:32:18Z

I will try to commit an alternative solution.

Done.

Basically by removing the gray background originally used to distinguish them from standard toggles. This background is no longer necessary for indicating interactivity, and removing it results in a cleaner, more consistent interface.

And use it for adding the agm-inline-menu-toggle CSS class when needed.

Since toggle are using now the default caret.

dgdavid

Left just a couple of minor notes, but overall it looks really good.

Thanks for handling the creation of the MenuButton core component. It looks solid and should help a lot with centralizing the code for building PatternFly menus more easily.

dgdavid · 2025-04-15T15:54:43Z

web/src/components/core/MenuButton.tsx

+} from "@patternfly/react-core";
+import { _ } from "~/i18n";
+
+/** PF complains with ids generated by useId because they contain colons. Such ids cannot be used


Worth to mention that useId() is actually generating an identifier that is a valid HTML id attribute but not a valid CSS identifier. Unfortunately, PatternFly component is querying the node by using the querySelector JavaScript method without escaping the id (by using CSS.escape() for example).

https://github.com/patternfly/patternfly-react/blob/51f35a918102ae3dcaec8ee577cf6f2f578aea16/packages/react-core/src/components/Menu/Menu.tsx#L157

That's why just using the identifier returned by useId is crashing with

Uncaught SyntaxError: Failed to execute 'querySelector' on 'Element': '#:r20:' is not a valid selector. at MenuBase.handleDrilldownTransition (Menu.js:34:1)

dgdavid · 2025-04-15T15:57:57Z

web/src/components/core/MenuButton.tsx

+
+interface MenuButtonItemProps extends Omit<MenuItemProps, "direction" | "drilldownMenu"> {
+  items?: React.ReactNode[];
+  upProps?: { label?: string };


In the future, I'd allow more MenuItem props for being able to influence such an item easily.

web/src/components/core/MenuButton.tsx

Co-authored-by: David Díaz <[email protected]>

Prepare to release Agama 14: * #1994 * #2041 * #2103 * #2178 * #2189 * #2200 * #2205 * #2209 * #2212 * #2213 * #2214 * #2215 * #2216 * #2217 * #2219 * #2220 * #2224 * #2225 * #2226 * #2227 * #2228 * #2230 * #2231 * #2232 * #2233 * #2235 * #2237 * #2239 * #2241 * #2242 * #2244 * #2245 * #2246 * #2247 * #2248 * #2249 * #2250 * #2251 * #2252 * #2253 * #2254 * #2255 * #2256 * #2257 * #2259 * #2260 * #2262 * #2265 * #2266 * #2268 * #2269 * #2271 * #2272 * #2273 * #2275 * #2276 * #2278 * #2281

https://build.opensuse.org/request/show/1272125 by user IGonzalezSosa + anag_factory - Version 14 - Process the product changed event so the UI can better react on registering the product shortly after that (gh#agama-project/agama#2274) - Rework the network page to (gh#agama-project/agama#2247): - Fix several problems with Wi-Fi networks handling. - Improve error reporting when the connection failed. - Use a regular form to connect, instead of the old "drawer" component. - Drop the "disconnect" and "forget" actions by now. - Reduce the amount of requests to the backend. - Shorten storage device names. - Move LVM logical volumes to partitions, if possible. - Add MenuButton component (gh#agama-project/agama#2241). - Improve the interface to display and edit the iSCSI initiator (bsc#1239046, bsc#1231385, gh#agama-project/agama#2269). - Fix flickering i

ancorgs added 4 commits April 2, 2025 11:07

web: Use basename instead of full name for disks

0b63216

web: Modify behavior of button to delete VG

700f8e5

web: WIP New DriveDeviceMenu component

0b9ee7e

web: Truncate long device names (in a suboptimal way)

a7ab48a

ancorgs changed the title ~~Reorganize the drive menus~~ Reorganize and redefine some storage menus Apr 4, 2025

joseivanlopez added 5 commits April 14, 2025 11:59

web: add component MenuButton

93bad18

- Unit tests still pending

web: adapt ConfigureDeviceMenu to use MenuButton

142888c

web: adapt DriveDeviceMenu to use MenuButton

dcc5e38

web: adapt tests

4063b9f

fix(web): get volume group targets correctly

cf62f73

joseivanlopez force-pushed the lvm-menus branch from 3f6f101 to cf62f73 Compare April 14, 2025 11:03

joseivanlopez added 7 commits April 15, 2025 07:01

web: save the active menu item

ab361d4

- The menu sets the aria-current property to the expanded item. - Useful for unit tests to detect whether a submenu is expanded. - Note that #toBeVisible cannot be used because Jsdom does not report correct styles, see jsdom/jsdom#2986.

web: add MenuButton tests

9d147ac

Merge branch 'master' into lvm-menus

8a69a41

web: small code improvement

47f7c07

- Avoid repeating the size of the drive name.

web: use menu item label for back label

0f52cfe

web: indicate whether the disk cannot be changed

fae3dbd

web: add aria-label to nested menus

0e06ef9

joseivanlopez force-pushed the lvm-menus branch from b6d93cc to 0e06ef9 Compare April 15, 2025 10:02

ancorgs commented Apr 15, 2025

View reviewed changes

web/src/components/storage/ConfigureDeviceMenu.tsx Outdated Show resolved Hide resolved

ancorgs added 2 commits April 15, 2025 14:25

web: Revisit truncation of device names

0ef60e9

web: Restore the generic string to go back on menus

4311e24

This kind of reverts 0f52cfe

dgdavid added 5 commits April 15, 2025 14:40

web: clean up inline toggle styles

6c1fb64

Basically by removing the gray background originally used to distinguish them from standard toggles. This background is no longer necessary for indicating interactivity, and removing it results in a cleaner, more consistent interface.

web: allow passing props to MenuButton toggle

53e59e4

And use it for adding the agm-inline-menu-toggle CSS class when needed.

web: drop no longer needed toggle tweak

faf8731

Since toggle are using now the default caret.

web: shorten CSS classname

51f02ae

web: use disabled color for disabled danger action

2ef3721

dgdavid approved these changes Apr 15, 2025

View reviewed changes

joseivanlopez marked this pull request as ready for review April 16, 2025 05:36

This comment was marked as resolved.

Sign in to view

Improve MenuButtonItem code

6ba69cd

Co-authored-by: David Díaz <[email protected]>

joseivanlopez force-pushed the lvm-menus branch from 4c3ee3e to 6ba69cd Compare April 16, 2025 06:01

joseivanlopez added 3 commits April 16, 2025 07:07

web: make clear why useId cannot be used

02e8a24

web: changelog

8fc5d55

Merge branch 'master' into lvm-menus

86fd1d1

ancorgs merged commit cdcf13e into agama-project:master Apr 16, 2025
1 check passed

imobachgs mentioned this pull request Apr 22, 2025

Update to release 14 #2282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reorganize and redefine some storage menus #2241

Reorganize and redefine some storage menus #2241

Uh oh!

ancorgs commented Apr 2, 2025 •

edited

Loading

Uh oh!

ancorgs commented Apr 15, 2025

Uh oh!

Uh oh!

ancorgs commented Apr 15, 2025

Uh oh!

dgdavid left a comment

Uh oh!

dgdavid Apr 15, 2025

Uh oh!

dgdavid Apr 15, 2025

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Reorganize and redefine some storage menus #2241

Reorganize and redefine some storage menus #2241

Uh oh!

Conversation

ancorgs commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Implementation details

Pending

About device names

Uh oh!

ancorgs commented Apr 15, 2025

Uh oh!

Uh oh!

ancorgs commented Apr 15, 2025

Uh oh!

dgdavid left a comment

Choose a reason for hiding this comment

Uh oh!

dgdavid Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

dgdavid Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ancorgs commented Apr 2, 2025 •

edited

Loading