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

Storage redesign #19472

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Oct 12, 2023

Overview demo: https://www.youtube.com/watch?v=aNS_4BejTDs
Mobile demo: https://www.youtube.com/watch?v=Gq7gBY-CciY

Polish demos:
Disabled menu items: https://www.youtube.com/watch?v=AQ9VU-FnzXs
Dangerous actions into menus: https://www.youtube.com/watch?v=RD9kMnKZ420
No "/dev/" prefix in the table: https://www.youtube.com/watch?v=FlD9OIgdxRM
Icons: https://www.youtube.com/watch?v=eLT_s5Y7Vaw
Different icons, "Size" header, indentation: https://www.youtube.com/watch?v=QrL3Pk9ZYvg
Raid stopping: https://youtu.be/U5gEZgY3N4k
Cards right side up: https://youtu.be/diSU7Hwt20Q
Fewer cards, no arrows: https://www.youtube.com/watch?v=lvyyiIh3aIg
Kezboard nav: https://www.youtube.com/watch?v=2lAcG-gouWw

Old demos:
https://youtu.be/aHir7PE9i2s
https://youtu.be/Mt7CkeDC9mQ
https://youtu.be/YwKPY3xjdPk
https://www.youtube.com/watch?v=zJHDXO3wVa4

Release notes

The Storage page has been redesigned with the goal of making more things visible more of the time. The overview page now shows all storage objects in a table and allows almost all operations to be performed on them right from the table.

Clicking on a row navigates to a page dedicated to the object in that row, with more information and maybe a few more actions.

We will continue to improve the Storage pages, by adding filtering options to the big table and working on the actions and dialogs.

image

@mvollmer mvollmer added no-test For doc/workflow changes, or experiments which don't need a full CI run, blocked Don't land until something else happens first (see task list) labels Oct 12, 2023
pkg/storaged/pages/drive.jsx Fixed Show fixed Hide fixed
pkg/storaged/pages/other.jsx Fixed Show fixed Hide fixed
pkg/storaged/create-pages.jsx Fixed Show fixed Hide fixed
pkg/storaged/create-pages.jsx Fixed Show fixed Hide fixed
pkg/storaged/create-pages.jsx Fixed Show fixed Hide fixed
test/verify/check-storage-basic Fixed Show fixed Hide fixed
test/verify/check-storage-ignored Fixed Show fixed Hide fixed
pkg/storaged/pages/drive.jsx Fixed Show fixed Hide fixed
pkg/storaged/create-pages.jsx Fixed Show fixed Hide fixed
@garrett
Copy link
Member

garrett commented Nov 27, 2023

Issues:

  • Back and forward do not work. (I clicked on a partition and when I hit back, I'm taken to the last page I was on, like Networking; I would've expected to see the overview instead.)
  • Accessibility: There's no relationship between partitions and their drives. Either the children table items would need to be nested, or we need to sprinkle some ARIA to indicate the relationship between them.
  • Mobile: doesn't show the parent-child relationship yet.

@mvollmer mvollmer force-pushed the storage-redesign branch 2 times, most recently from a177612 to 262c829 Compare November 29, 2023 07:46
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

phew, I walked through all the pkg/storaged/ changes, I'll do a separate review for the tests. First of all, I like the file/class/function structure -- it's very symmetric, which helps with reviewing. I ignored (almost) all of JS code style issues, the dialogs, and the Reacty/design bits, and mostly looked for odd/unexplained/magic bits.

I realize that this bitrots fast, so I'm happy for most/all of my comments to be addressed in a follow-up. I'm just asking to not land this in today's release, so that we have two weeks for follow-ups.

pkg/storaged/block/create-pages.jsx Outdated Show resolved Hide resolved
pkg/storaged/block/other.jsx Outdated Show resolved Hide resolved
pkg/storaged/drive/drive.jsx Outdated Show resolved Hide resolved
pkg/storaged/filesystem/filesystem.jsx Show resolved Hide resolved
pkg/storaged/mdraid/mdraid.jsx Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
pkg/storaged/pages.jsx Show resolved Hide resolved
pkg/storaged/pages.jsx Outdated Show resolved Hide resolved
pkg/storaged/pages.jsx Outdated Show resolved Hide resolved
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Tests are fairly straightforward, but they may have spotted a bug. Thanks for the careful pixel coverage!

test/verify/check-storage-mdraid Show resolved Hide resolved
@mvollmer mvollmer added no-test For doc/workflow changes, or experiments which don't need a full CI run, and removed no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Nov 29, 2023
@mvollmer
Copy link
Member Author

Coverage notes:

  • LV renaming
  • Activation of volume(?)
  • Removal of non-empty PV
  • volume group polling
  • Starting pool from overview, pixel test for dialog

martinpitt
martinpitt previously approved these changes Nov 30, 2023
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Cheers! I reviewed the fixups since my last review, and updated the review threads. There are no more blockers from my side, and tests are green (f39 appears red because of a github flake with the API, but it did succeed).

I leave the final sign-off to @garrett , for the remaining discussions about icons and such.

Looking forward to seeing this land! 🏁

@garrett
Copy link
Member

garrett commented Nov 30, 2023

I can't navigate by keyboard.

  • How to you select a drive or partition to see more or know what type it is via keyboard focus (without a mouse)?
  • How do you select actions in the menu by keyboard alone?

(I can focus a row and also focus the menu for each row, but I can't activate them.)

This is important for accessibility.

@garrett
Copy link
Member

garrett commented Nov 30, 2023

Icons don't have enough contrast in dark mode. (They need to be lighter.)

Edit: Fixed!

martinpitt
martinpitt previously approved these changes Dec 1, 2023
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

I additionally reviewed the kezboard navigation, and while it is "ugh we need to do that?", it technically looks fine to me. Thanks!

Co-authored-by: Garrett LeSage <[email protected]>
@martinpitt martinpitt merged commit f0b0961 into cockpit-project:main Dec 1, 2023
91 of 93 checks passed
@teg
Copy link

teg commented Dec 1, 2023

Amazing work! Thanks everyone!

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.

5 participants