Storage UI: Delete Partition (in proposal; trash-can icon) by mvidner · Pull Request #1915 · agama-project/agama

mvidner · 2025-01-17T12:23:49Z

Problem

Making the trash-can icon work

https://trello.com/c/vPPaRAIg

Solution

delete the item from the list (via configModel.Config)
~~ask for confirmation~~
~~check that the partition is not mandatory~~ ... these two are postponed to a followup PBI

Testing

Tested manually
Added a unit test for the UI part (thanks David!)

Screenshots

No. The deleted partition is not visible 🤣 and a video would be overkill.

WIP

that is, without asking for confirmation or checking if it is mandatory. split out PartitionMenuItem, because eslint said: 607:40 error React Hook "usePartition" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function react-hooks/rules-of-hooks

apparently a copy-paste bug in the prototype BTW shouldn't we use _(translations) more?

web/src/queries/storage/config-model.ts

mvidner · 2025-01-22T09:27:32Z

I haven't reviewed all the code again, but if you don't mind would be nice if you add a unit test for ensuring the interface behaves as we expect.

BTW I noticed that the ci-web.yml job does have a npm test -- --coverage --silent step, but we also used to upload the result to coveralls.io which is now commented out. Perhaps it is time to bring it back?

dgdavid · 2025-01-22T12:31:35Z

I haven't reviewed all the code again, but if you don't mind would be nice if you add a unit test for ensuring the interface behaves as we expect.

BTW I noticed that the ci-web.yml job does have a npm test -- --coverage --silent step, but we also used to upload the result to coveralls.io which is now commented out. Perhaps it is time to bring it back?

I think so. In fact, I asked for it back in September 12th at internal #team-yast Slack channel 😉 (see https://suse.slack.com/archives/C06JYR4ARRC/p1726172205760599)

web/src/components/storage/DriveEditor.tsx

dgdavid · 2025-01-23T01:55:35Z

@mvidner I have updated the code and the test. Find some random comments below.

About your doubts written as comments in the test, you were right. There are a lot of missing pieces in the interface concerning a11y. That’s somewhat intentional for the first round of this new proposal. It’s not the ideal way to work, since a11y should be another success criterion for a good MVP, but…

Fortunately, RTL helps a lot in spotting an interface with markup issues due to its user interaction-based testing approach. Please, have a look at the changes I made and let’s discuss them in a meeting if needed. I’ve only done a few basic things to ensure we can at least test it easily, but there’s more work ahead (as follow-up tasks, I mean).

As an aside note, think of these aria-labels as the missing labels for these selectors. We’ve used sentences to provide context for sighted people, but we need to think of a good way to provide similar information with the same interaction fluency for screen readers.

Another thing, not related to your PR, is the number of components inside the DriveEditor file. They should be exported in order to test them in isolation, for unit testing. Also, we need to review the entire file to reduce component nesting as much as possible, which might result in splitting it into a couple of files to group the pieces that are part of the DriveEditor thingy.

Finally, it would be great if you could rebase the storage-config-ui branch before proceeding with the merge, as quite some things have changed. Don’t forget to run npm install after doing so.

and use `deletePartition` as variable name not to collide with the `delete` operator

having to set up mocks for the whole DriveEditor is not what I expected :-/

Markup for adding roles and labels to make the interface a bit more accessible and easing unit testing with React Testing Library. Still needing love, thougth. But this is a kind of starting point / guidance commit.

coveralls · 2025-01-23T13:15:57Z

Pull Request Test Coverage Report for Build 12929888022

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 72.423%

Totals
Change from base Build 12887288857:	0.0%
Covered Lines:	19011
Relevant Lines:	26250

💛 - Coveralls

ancorgs · 2025-01-23T14:22:00Z

web/src/queries/storage/config-model.ts

+};
+
+// driveName, like "/dev/sda"
+// mountPath, like "/" or "swap"


It makes sense to follow here the same syntax (jsdoc?) we are using all along the rest of the code.

So this could be the first properly documented function in this file.

ancorgs · 2025-01-23T14:37:38Z

web/src/components/storage/DriveEditor.test.tsx

+      mountPath: "swap",
+      size: {
+        min: 2_000_000_000,
+        default: false, // WTF does default mean??


If the size was omitted and, as a result, that min of 2_000_000_000 was calculated by Agama itself, then "default" is true.

On the other hand, if that value of 2_000_000_000 was provided by the user, "default" is false.

ancorgs

Generally looks good. I added a comment about documentation and a clarification about a question raised in a comment.

Other than that, I only miss the changelog entry.

even better it belongs to storage.model.schema.json

mvidner · 2025-01-23T15:21:43Z

web/src/components/storage/DriveEditor.test.tsx

+      mountPath: "swap",
+      size: {
+        min: 2_000_000_000,
+        default: false, // false: user provided, true: calculated


@ancorgs thanks for the explanation...
now I know that the comment should actually go to storage.model.schema.json, but I haven't found a Makefile-like declaration to update the generated config-model.ts so I am postponing this yak shave

ancorgs

LGTM

…his device) (#1934) ## Problem https://trello.com/c/JQ3blpRm ![remove](https://github.com/user-attachments/assets/0b714e90-dacf-46f9-8ee4-e6e863a176ae) ## Solution The implementation was easier because the UI model already had a `removeDrive` method... but testing was fun again, because imitating the tests that worked for deleting a partition was not enough this time 😅 ## Testing - Added a new unit test - Tested manually ## Screenshots In action (used [Peek](https://github.com/phw/peek) for recording, thanks Ancor for the tip) 1. Try to delete `nvme01` but not possible because it contains `/` 2. Delete `/` from it (#1915)... (and we have an issue box, that's new but not from me 😄 ) 3. Now can delete `nvme01` ![agama-delete-proposed-partition-and-drive](https://github.com/user-attachments/assets/81ae11b2-6fd3-4921-8d6e-d423af94cb1a)

Prepare for releasing Agama 12: * #1858 * #1887 * #1890 * #1892 * #1893 * #1894 * #1896 * #1898 * #1899 * #1900 * #1901 * #1906 * #1908 * #1909 * #1910 * #1911 * #1912 * #1914 * #1915 * #1917 * #1919 * #1920 * #1921 * #1922 * #1923 * #1924 * #1926 * #1927 * #1928 * #1929 * #1930 * #1931 * #1932 * #1933 * #1934 * #1935 * #1936 * #1937 * #1938 * #1939 * #1942 * #1943 * #1945 * #1948 * #1949 * #1952 * #1953 * #1954 * #1955 * #1957 * #1958 * #1959 * #1961 * #1963 * #1964 * #1967 * #1969 * #1970 * #1971 * #1972 * #1973 * #1974 * #1975 * #1976 * #1977 * #1979 * #1980 * #1981 * #1982 * #1984 * #1986 * #1987 * #1988 * #1990 * #1991 * #1992 * #1993 * #1995 * #1996 * #1997 * #1999 * #2000 * #2001 * #2002 * #2003 * #2004 * #2005 * #2006 * #2007 * #2008 * #2009 * #2010 * #2011 * #2012 * #2013 * #2014 * #2015 * #2016 * #2017 * #2019 * #2021 * #2022 * #2025 * #2027 * #2029 * #2030 * #2031 * #2033 * #2034 * #2035 * #2036 * #2037 * #2038 * #2039 * #2040 * #2045 * #2046 * #2050 * #2053 * #2054 * #2055 * #2056 * #2058 * #2060 * #2061 * #2062 * #2063 * #2064 * #2066 * #2067 * #2068 * #2069 * #2070 * #2071 * #2072 * #2073 * #2075 * #2076 * #2080 * #2082 * #2083

mvidner changed the title ~~WIP just log onClick the Delete icon~~ Storage UI: Delete Partition (in proposal; trash-can icon) Jan 17, 2025

mvidner added 3 commits January 17, 2025 15:58

just log when the UI button is clicked

fbe38a2

WIP

fix aria-label=Delete

f2f32f3

apparently a copy-paste bug in the prototype BTW shouldn't we use _(translations) more?

mvidner force-pushed the delete-proposed-partition branch from bfc96fb to f2f32f3 Compare January 17, 2025 15:10

mvidner marked this pull request as ready for review January 17, 2025 15:11

dgdavid reviewed Jan 21, 2025

View reviewed changes

web/src/queries/storage/config-model.ts Outdated Show resolved Hide resolved

web/src/queries/storage/config-model.ts Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

dgdavid reviewed Jan 22, 2025

View reviewed changes

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

mvidner mentioned this pull request Jan 22, 2025

Storage UI: Delete Drive (in proposal; Remove the configuration for this device) #1934

Merged

dgdavid force-pushed the delete-proposed-partition branch from 71cbcc7 to 9c94e8b Compare January 23, 2025 01:34

dgdavid force-pushed the delete-proposed-partition branch from 9c94e8b to 5f012bb Compare January 23, 2025 02:06

mvidner and others added 3 commits January 23, 2025 10:42

rename to PartitionHook.delete

104195f

and use `deletePartition` as variable name not to collide with the `delete` operator

WIP test

b64ff55

having to set up mocks for the whole DriveEditor is not what I expected :-/

fix(web): extend partitions menu markup and add a test

f292d2c

Markup for adding roles and labels to make the interface a bit more accessible and easing unit testing with React Testing Library. Still needing love, thougth. But this is a kind of starting point / guidance commit.

mvidner force-pushed the delete-proposed-partition branch from 5f012bb to f292d2c Compare January 23, 2025 09:55

mvidner added 2 commits January 23, 2025 13:52

Merge branch 'storage-config-ui' into delete-proposed-partition

8f91158

adjust test after merging with upstream

2d8b27e

ancorgs reviewed Jan 23, 2025

View reviewed changes

mvidner added 3 commits January 23, 2025 16:06

comment usePartition better

84c37e0

comment Size.default better

3c52b08

even better it belongs to storage.model.schema.json

changelog

50f677d

mvidner commented Jan 23, 2025

View reviewed changes

ancorgs approved these changes Jan 23, 2025

View reviewed changes

mvidner merged commit 8d0d533 into storage-config-ui Jan 23, 2025

mvidner deleted the delete-proposed-partition branch January 23, 2025 21:00

imobachgs mentioned this pull request Feb 26, 2025

Update to release 12 #2084

Merged

Conversation

mvidner commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Screenshots

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

mvidner commented Jan 22, 2025

Uh oh!

dgdavid commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dgdavid commented Jan 23, 2025

Uh oh!

coveralls commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12929888022

Details

💛 - Coveralls

Uh oh!

ancorgs Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

ancorgs Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

ancorgs Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

ancorgs left a comment

Choose a reason for hiding this comment

Uh oh!

mvidner Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

ancorgs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvidner commented Jan 17, 2025 •

edited

Loading

dgdavid commented Jan 22, 2025 •

edited

Loading

coveralls commented Jan 23, 2025 •

edited

Loading