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

machines: replace os-vendor/os dropdowns with TypeAheadSelect component from PF-react #12152

Merged
merged 7 commits into from
Aug 1, 2019

Conversation

KKoukiou
Copy link
Contributor

os-not-found
fedora-filter
os-spinner

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.

Looks fine, nice cleanup! Looking forward to test this in action. Two questions, and can you please rebase?

@martinpitt
Copy link
Member

A single list will mean that it's harder to operate with just the mouse, as it will be a looong merged list, right? Is that an a11y concern?

(I didn't yet test this, waiting for the rebase)

@KKoukiou
Copy link
Contributor Author

A single list will mean that it's harder to operate with just the mouse, as it will be a looong merged list, right? Is that an a11y concern?

(I didn't yet test this, waiting for the rebase)

On the other hand you are able to filter results by typing some substring of the desired distro.
Please test, and if you still don't like it let's drop this.

@martinpitt
Copy link
Member

On the other hand you are able to filter results by typing some substring of the desired distro.

Right, I personally like this a lot, but I'm a very keyboard-affine person. I was just wondering what that means for mouse users.

Also, right now I can type arbitrary strings into that box, like "foo", and there is no error message. It should only be allowed to select an existing entry.

@garrett
Copy link
Member

garrett commented Jun 26, 2019

This is problematic as-is.

I discussed this with @andreasn and have an idea on how to incorporate this into a working design which would allow:

  • automatic matching
  • type to search
  • browsability

It would basically be two side-by-side components in the dialog.

I don't have time right now to work on a proper mockup, but I'm adding a table-based sketch on what I have in mind. The first column is the label. The second is a select with several options. The third is either a text string for the auto-detect, an input and auto-complete list for search, or a second dropdown for OS version (when browsing based on vendor on the first dropdown)

Label Primary (Dropdown)  Secondary (String / Input / Dropdown)  
Operating System Auto-detect Determining OS…
    Fedora 30
     
  Search [_________]
    (list of matches when search field is used)
     
  Red Hat Supported
    - Red Hat Enterprise Linux 8
    - Red Hat Enterprise Linux 7
    Extended Support
    - Red Hat Enterprise Linux 6
    - Red Hat Enterprise Linux 5
    End of Life
    - Red Hat Enterprise Linux 4
    - Red Hat Enterprise Linux 3
    - Red Hat Enterprise Linux 2
    - Red Hat Enterprise Linux 1
     
  Fedora Supported
    - Fedora 30
    - Fedora Silverblue 30
    - Fedora 29
    - Fedora Silverblue 29
    End of Life
    - Fedora 28
    - Fedora Silverblue 28
     
 

It would default to auto-detection. If that doesn't work, the option becomes disabled and then the next selection option should be search, which has the widget from this PR. People can optionally browse by drilling down with vendors and distribution versions, which are then grouped by supportability.

Note: One thing I didn't mock-up is showing supportability in the search or even auto-detection... but this should be shown somehow.

@andreasn
Copy link
Contributor

I agree that this becomes very keyboard focused, and there need to be some way to manually select an OS, without knowing exactly what to type.
If we want to keep it to just one field, it would need to be something like this:
https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html

Just showing the entire list straight up and down would end up being quite a long list.
I wonder how long the list would become if we just listed OSes that are not EOL, and then we'll have some way of revealing the full list of the OSes (also EOLed ones)
Is there an easy way to query libosinfo for just the supported OSes easily via the CLI?

@andreasn
Copy link
Contributor

But if we go for the 2-field solution that @garrett proposes, do we really need "Auto-detect" as a dropdown? I think we could just show the right entry in the list and not make a big fuzz about it (just like how it works now), then you would be able to correct that selection, or go for the Search action.

@andreasn
Copy link
Contributor

Relevant info, recognition vs recall, see bullet point 3.

@garrett
Copy link
Member

garrett commented Jun 26, 2019

do we really need "Auto-detect" as a dropdown [...]?

How long does it take to check?

We could have a placeholder message and then replace it with a widget similar what I described if it fails to detect (except without autodetect). If it doesn't take long. And if it doesn't get it wrong.

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Jun 26, 2019

I agree that this becomes very keyboard focused, and there need to be some way to manually select an OS, without knowing exactly what to type.
If we want to keep it to just one field, it would need to be something like this:
https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html

Which of the mentioned examples you mean?

Just showing the entire list straight up and down would end up being quite a long list.

It's long, filtering with substrings helps only in this case.
Of course the component implements paging, the list is not presented to the user all at once.
(I understand your thought regarding that the list can get long, but on the other hand with the previous implementation we forced the user to go over two form fields to do basically one operation, select OS.)
I think that the previous implementation benefited more mouse users however this one benefits keyboard users, each of them with their drawbacks.
I lean to like more this one, because the Create VM dialog keeps growing in the amount of input fields, and merging inputs together makes sense to me.

I wonder how long the list would become if we just listed OSes that are not EOL, and then we'll have some way of revealing the full list of the OSes (also EOLed ones)

I don't think this component allows such logic to be implemented.
Keep in mind that I am using an patternfly component, the flexibility here is not so big.
I can only pass to it the list of entries that it's supposed to present.

Is there an easy way to query libosinfo for just the supported OSes easily via the CLI?

It's possible to query osinfo-db for EOF for each OS.

@KKoukiou
Copy link
Contributor Author

do we really need "Auto-detect" as a dropdown [...]?

How long does it take to check?

It's a matter of 1-2 seconds after inserting the installation source.
There is a spinner shown when the component is trying to detect the OS.

We could have a placeholder message and then replace it with a widget similar what I described if it fails to detect (except without autodetect). If it doesn't take long. And if it doesn't get it wrong.

It can't really get it wrong, it can only not get it at all.
I can replace the placeholder when it's searching. That's not a problem.

@andreasn
Copy link
Contributor

I agree that this becomes very keyboard focused, and there need to be some way to manually select an OS, without knowing exactly what to type.

This was false actually. I didn't have libosinfo installed and didn't realize I needed to.
So selecting the input field will give you a list of OSes. I couldn't quite figure out how this list is ordered, and it seems a bit random to me, because I get a dozen versions of FreeBSD at the top, followed by Fedora, then Ubuntu and then a dozen DragonflyBSD. So it seems I am able to browse for the OSes, but the sorting doesn't make sense to me.

@KKoukiou
Copy link
Contributor Author

I agree that this becomes very keyboard focused, and there need to be some way to manually select an OS, without knowing exactly what to type.

This was false actually. I didn't have libosinfo installed and didn't realize I needed to.

You don't have libosinfo installed because you built cockpit-machines from sources and you don't have its dependencies.
For normal users it will be there.

So selecting the input field will give you a list of OSes. I couldn't quite figure out how this list is ordered, and it seems a bit random to me, because I get a dozen versions of FreeBSD at the top, followed by Fedora, then Ubuntu and then a dozen DragonflyBSD. So it seems I am able to browse for the OSes, but the sorting doesn't make sense to me.

Probably the order get's messed up somewhere indeed.
I will adjust to preserve the order that osinfo-query gives to us
osinfo-query os --fields=name,eol-date | tail -n +3 | sed -e 's/\s*|\s*/|/g; s/^\s*//g; s/\s*$//g'

@andreasn
Copy link
Contributor

It seems I can also write in anything in freeform now. I tried writing "lennart" and it happily accepted that. Should we check that whatever is written actually matches with a real OS name? Otherwise there is the risk you'll get weird results because someone mistypes "Windows Server 2012" as "Qindows Aerver 2012" or stuff like that and the installation fails due to not setting some flag right.

@KKoukiou
Copy link
Contributor Author

It seems I can also write in anything in freeform now. I tried writing "lennart" and it happily accepted that. Should we check that whatever is written actually matches with a real OS name? Otherwise there is the risk you'll get weird results because someone mistypes "Windows Server 2012" as "Qindows Aerver 2012" or stuff like that and the installation fails due to not setting some flag right.

Yes, this issue is already mentioned here #12152 (comment), I am going to address it if we agree to use this approach.

@KKoukiou KKoukiou added the blocked Don't land until something else happens first (see task list) label Jul 23, 2019
@KKoukiou KKoukiou removed the blocked Don't land until something else happens first (see task list) label Jul 23, 2019
KKoukiou and others added 7 commits July 31, 2019 14:07
…nt from PF-react

Having two entries for information that can be fetched with
one element is not user friendly.
This commit simplifies the OS related entries in Create VM dialog,
by replacing the dropdowns with a TypeAheadSelect component from
PF-react.

The new component has better support for indicating that the OS is being
autodetected by showing a spinner.

Closes cockpit-project#12152
All OS data are coming from libosinfo but until this commit we had this
extra 'Other OS' attribute appearing on list.
Since this is not original data from libosinfo just drop it.
Users who don't want to specify an OS will just not autofill the OS
dialog field.
Always hide OSes from the list which are either with too old release
date or too old EOL date.

Tests were adjusted to not try to use OSes which will not be included in
the list anymore.
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.

Thanks! This works very well now, I didn't manage to break it any more.

@martinpitt martinpitt dismissed marusak’s stale review August 1, 2019 14:13

tests seem fine now, kkoukiou answered about your review comments

@martinpitt martinpitt merged commit a21e11b into cockpit-project:master Aug 1, 2019
martinpitt pushed a commit that referenced this pull request Aug 1, 2019
…nt from PF-react

Having two entries for information that can be fetched with
one element is not user friendly.
This commit simplifies the OS related entries in Create VM dialog,
by replacing the dropdowns with a TypeAheadSelect component from
PF-react.

The new component has better support for indicating that the OS is being
autodetected by showing a spinner.

Closes #12152
@KKoukiou KKoukiou deleted the typeahead-os branch August 1, 2019 14:27
@KKoukiou
Copy link
Contributor Author

KKoukiou commented Aug 1, 2019

In case we include it in release notes:
Screen Shot 2019-08-01 at 16 27 29

@garrett
Copy link
Member

garrett commented Aug 1, 2019

Here's a cropped (with 25px margin) and optimized version of the screenshot:

62301673-50bb3780-b479-11e9-9356-ea01e65f0c86-cropped (copy)

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