Revert "storage: Expose Stratis virtual filesystem sizes" #21039
Conversation
This reverts commit 236ffd3. It went in without approval.
|
@garrett, fyi. |
|
Thanks. I see in matrix that a release already happened. Can we do a hotfix, and can we please try to prevent this kind of thing from happening somehow? I guess a lot of people won't even see the feature unless Stratis is enabled on their system, at least? |
|
Removing release blocker as the release happened. |
|
Adding release blocker as there is always the next release. |
|
My gut feeling FWIW: revert and killing the release feels too strong. The wording should be updated (No "Manage" and remove the redundant "virtual filesystem" at least), but that smells more like a follow-up than a revert? |
|
It also needs to go somewhere else. In-between name and mountpoint doesn't make sense. |
|
The problem isn't that this needs changes, the problem is that it should've had design considerations at the start and that it should've changed before being merged. The problem is that this is a repeating pattern that we need to fix. This wasn't so much of a problem when I watched every single change done within Cockpit, but that was a huge frustrating timesink (especially since there's just me and not myself and Andreas to work on everything), so I had to taper that back (I still see too much in GitHub notifications, but not as much) and have relied on everyone in the team to actually contact me when the UI or UX will change at all. But people in the team keep dropping the ball on this. If something changes the way it looks or acts, design needs to be considered, up front. And it should have iterations to fix issues related to design before it gets merged. This is the problem, not wording, not "CSS", not layout. Wording and where it goes are definitely problems here (and thankfully small problems in this feature), but not the fundamental issue. Thankfully, in this case, it is probably a quick fix. But we need to — as a complete team — make sure that things that need design consideration actually get design consideration, throughout the entire feature. I can't be alone in doing this; everyone needs to work together on this. This is an equivalent to merging in a PR without having any code review, linting, and tests, and I think everyone on the team would frown on that, right? |
|
Again: This in particular is probably specifically a simple fix, and it slipped under the radar. That's understandable; it'll happen. (The problem is that this happens all too often lately.) Also: I don't know how to test this or how to see what happens when boxes are checked however. I assume if you check on limiting the size, you'd give it the size? |
|
Also: I do appreciate this being sent as a revert with a release blocker. I converted this to draft to prevent it from being merged, as in this case, we can probably do a simple hotfix, thankfully. (I'm assuming, as I can't see the full impact of this from a single screenshot that doesn't show everything the feature can do.) |
|
This is the current screenshot:
It's from #20611 Probably something like this:
But it's hard to know, without more screenshots or a clear way to test this out locally. Also, I'm assuming none of this is visible when you don't have Stratis already set up? |
FTR, I find it more logical to first specify the size and then the mount point -- that matches planning and chronological order. Agreed to the wording changes, thanks! |
|
I'm looking at the videos. This is a UI mess.
Videos @ https://www.youtube.com/watch?v=E1zqhAowus8 and https://www.youtube.com/watch?v=1W6QPNPwT7Q
There are more labels too. This needs more work. I'm working on retroactive mockups now, to try to clean this up, trying to do so with as little invasive changes as possible, but it'll take some changes. This feature is not done, and this needs to be fixed. This should've been fixed before it was implement, and definitely before it was merged. |
OK, I'll try to work that in and see. |
|
So, using the exact same UI, but cleaned up to conform to basic design principles (including what PF docs mention), it would look like this by default:
And when you expand, you get progressive disclosure as a part of the item, as PatternFly mentions in the docs and that we have done everywhere else in Cockpit for more than a decade:
The widths should not stretch across, unless they need to contain a lot of information (like a path, and the name could possibly also). Widths should be approximate to how much information is needed within. If we don't have upper and lower bounds, then the sliders do not make sense. We would use a number-constrained text input instead, or a number widget if it's supposed to be able to be incremented in steps. Dropdowns for sizes should not be set to minimal numbers (MB, KB, or bytes), but should default to something that is a realistic unit instead (GB, for example). They should be the same unit by default for both entries. We could probably improve the design further, but this should be rather a set of rather basic, minimal changes to what we have. Labels might be a little verbose, but still straightforward. I'd prefer shorter strings, but I fear they might not be as straightforward?
Perhaps if we changed the group label to "Virtual filesystem" and let the size on the checkbox labels speak for it by context, that would work better? |
|
Here's what that would look like:
And if we just say "Stratis filesystem":
(The target audience knows it's Stratis and wouldn't necessarily understand that virtual filesystem is a Stratis filesystem, so it's probably fine to include this jargony name here.) |
Clean up the dialog changes from commit 236ffd3 as per [1]: - Visually merge both options into "Stratis filesystem" and avoid extra top-level labels. - Point out that the sizes are Stratis specific, as it doesn't say anywhere else in the dialog. - Remove redundant "virtual filesystem" words in the options - Avoid "Manage", pretty much all of Cockpit is "managing" something. [1] cockpit-project#21039 (comment)
|
I sent PR #21046 to start implementing this design. In the spirit of "gradual improvement". |
Clean up the dialog changes from commit 236ffd3 as per [1]: - Visually merge both options into "Stratis filesystem" and avoid extra top-level labels. - Point out that the sizes are Stratis specific, as it doesn't say anywhere else in the dialog. - Remove redundant "virtual filesystem" words in the options - Avoid "Manage", pretty much all of Cockpit is "managing" something. [1] cockpit-project#21039 (comment)
Clean up the dialog changes from commit 236ffd3 as per [1]: - Visually merge both options into "Stratis filesystem" and avoid extra top-level labels. - Point out that the sizes are Stratis specific, as it doesn't say anywhere else in the dialog. - Remove redundant "virtual filesystem" words in the options - Avoid "Manage", pretty much all of Cockpit is "managing" something. [1] #21039 (comment)
Clean up the dialog changes from commit 236ffd3 as per [1]: - Visually merge both options into "Stratis filesystem" and avoid extra top-level labels. - Point out that the sizes are Stratis specific, as it doesn't say anywhere else in the dialog. - Remove redundant "virtual filesystem" words in the options - Avoid "Manage", pretty much all of Cockpit is "managing" something. [1] cockpit-project#21039 (comment)
This reverts commit 236ffd3.
It went in without approval.