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: re-design the disk selection component #440

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Sep 13, 2024

Move disk selection from the inline select inside a modal dialog. Move the re-scan button also there to make the disk selection entry page more minimal.

Screen Shot 2024-09-13 at 16 56 28

Screen Shot 2024-09-13 at 16 58 46

@KKoukiou KKoukiou force-pushed the disk-selection-redesign branch 6 times, most recently from 265e335 to 706b694 Compare September 13, 2024 14:36
@KKoukiou KKoukiou marked this pull request as ready for review September 13, 2024 15:00
@garrett
Copy link
Contributor

garrett commented Sep 16, 2024

To confirm, the modal, when it gets more content (like a lot of disks), grows taller and the box scrolls, like in this horribly quick mockup based on the screnshot:

image

@jkonecny12 was concerned about this on Slack, but this is what those widgets should do by default. The list means it overflows, and the modal should be able to grow with more content.

If it doesn't do this, then that's a bug.

@jkonecny12
Copy link
Member

Looks good to me design wise.

@KKoukiou KKoukiou mentioned this pull request Sep 16, 2024
10 tasks
@garrett
Copy link
Contributor

garrett commented Sep 16, 2024

Design related issues:

  1. Spacing in the wizard page is not correct. There is too much space between the list and the edit link, and there's not enough space between sections.
  2. Rescan devices is supposed to affect the list, so it should be closer to the list, but there's too much space between it and the action buttons, which are unrelated.
  3. Cancel should not be a secondary button, but a link-styled button.
  4. The modal is probably a little too short by default. It should be taller and also should grow to accommodate more devices in a list. (See the mockup for approximate default sizing. See modified screenshot as a quick, thrown-together mockup to see what it'd look like with tons of devices.)

Here are the most recent versions of the mockups, for reference:

Installation method_ Free space_ enough, opt-in(2)

Installation method_ Free space_ enough, opt-in(4)

Here's how the modal would look with multiple, 2 items, and 1 item (where it becomes force-selected, unless another disk is added). The right two have a minimum height, but then it starts to grow and become overflowed with a scrollbar like the above modified screenshot I posted.

Board(23)

Copy link
Contributor

@garrett garrett left a comment

Choose a reason for hiding this comment

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

(see above for review)

@KKoukiou KKoukiou force-pushed the disk-selection-redesign branch 2 times, most recently from adf567b to 0bccc33 Compare September 17, 2024 07:57
@garrett
Copy link
Contributor

garrett commented Sep 17, 2024

There's currently no way to refresh when there are no disks, and the actions should be just a "Close" button in secondary style. (You cannot select nothing, and you cannot cancel... just close, unless you click refresh and disks appear, then it should go back to "Select" and "Cancel".)

image

I can make a mockup for this state.

@garrett
Copy link
Contributor

garrett commented Sep 17, 2024

Probably something like one of these:

image

I like the top-left of these the most, but that'd mean we would need to change the modal like this:

image

(Whoops! I forgot to change the Select/Cancel to close! Fixing below...)

So the pairs of disks vs. no disks would look like this:

image

Which looks best to you?

I like the leftmost pair together overall, but as having no disks available is not a common situation, we could go with the rightmost (the one with the left alignment).

We could also drop the icon:

image

@garrett
Copy link
Contributor

garrett commented Sep 17, 2024

The most straightforward one to implement is probably this, which is probably fine, really, especially since nobody should see this error state (hopefully):

image

However, we do need a refresh in the no disks mode due to the interactive tests, so we need this. (I'll probably always forget to add a disk before this, and without being able to refresh, it'll be annoying to have to restart a VM.)

@rvykydal
Copy link
Contributor

@KKoukiou should I wait with a review a bit (after finishing the design review?).

Move disk selection from the inline select inside a modal dialog.
Move the re-scan button also there to make the disk selection entry
page more minimal.
This affects the spacing between the sections, which according to
designer feedback was previously insufficient.
@KKoukiou
Copy link
Contributor Author

@KKoukiou should I wait with a review a bit (after finishing the design review?).

I think I addressed all design comments.

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@KKoukiou KKoukiou merged commit 6917fff into rhinstaller:main Sep 19, 2024
9 checks passed
@KKoukiou KKoukiou deleted the disk-selection-redesign branch September 19, 2024 13:45
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