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

create: Show all operating systems #1872

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Oct 23, 2024

This will show all operating systems, regardless of EOL or release dates, but the recommended ones will be at the top. Operating systems with an expired EOL date will be annotated accordingly in the list.

The tests have been brought up-to-date with regard to what OSes are actually in the list and are downloadable.

Fixes #509

Demo: https://youtu.be/I9VCtm2T49U

@mvollmer
Copy link
Member Author

This is a variant of #1869.

@martinpitt
Copy link
Member

Thanks! See discussion in #1869, I like both of them, I just can't quite make up my mind.

@garrett
Copy link
Member

garrett commented Oct 23, 2024

Specifically pointing out some issues in this screenshot crop:

image

  1. We have some problems, due to PF, where the sub-text and the headings are exactly the same.
  2. Some entries are grouped under old, but they don't show EOL, like AlmaLinux 8 there...
  3. Is AlmaLinux 8 actually old/EOL? It's available for download on https://almalinux.org/get-almalinux/ — or does that mean 8.0 specifically (instead of all 8.x), as 8.10 is available for download?

@mvollmer mvollmer removed the blocked label Oct 24, 2024
@mvollmer
Copy link
Member Author

Some entries are grouped under old, but they don't show EOL, like AlmaLinux 8 there...

AlmaLinux 8 was released on 2021-03-30 (according to osinfo anyway), which is more than 3 years ago. We should probably also show release dates in the "old" group:

image

Is AlmaLinux 8 actually old/EOL? It's available for download on https://almalinux.org/get-almalinux/ — or does that mean 8.0 specifically (instead of all 8.x), as 8.10 is available for download?

There is only "AlmaLinux 8" and "AlmaLinux 9" in the osinfo list. According to https://wiki.almalinux.org/release-notes/, it is actually AlmaLinux 8.3 that was released on 30 Mar 2021. When you install "AlmaLinux 8", you get AlmaLinux 8.10.

So, yeah, it's a bit messy.

@garrett
Copy link
Member

garrett commented Oct 24, 2024

This is probably the right approach, to use groups instead of the expander thingy. It's fewer interactions by default, and the grouping brings the important ones to the top when browsing and searching. (The other approach would still need to do this when searching.) The biggest downside to this approach is having a tinier scrollbar, which is mitigated when filtering the list.

Now that the old ones without EOL have a date there too, I think the main remaining thing we need to consider is to differentiate them, and I think the warning icon would make sense. Are we able to add that in PatternFly directly? (Otherwise, we might have to resort to a little bit of CSS to add the icon, if not.)

@mvollmer
Copy link
Member Author

Now that the old ones without EOL have a date there too, I think the main remaining thing we need to consider is to differentiate them, and I think the warning icon would make sense.

So a warning icon for those with EOL date, and no warning icon for those a release date?

@mvollmer
Copy link
Member Author

Like this?

image

@mvollmer
Copy link
Member Author

With description on the same line:

image

@mvollmer
Copy link
Member Author

The deprecated Select component has problems: It shows very poor search performance when isGrouped is used, and it somehow can't find entries that end in ")".

image

image

These bugs are not present without isGrouped. I have not tried the non-deprecared Select. I think porting to it just got more important.

@martinpitt
Copy link
Member

With description on the same line:

Nice on desktops, but does that work reasonably on mobile? Or is it some flex magic that automatically breaks it into two lines then?

@mvollmer
Copy link
Member Author

The search performance of the deprecated Select seems to be so bad that we get test timeouts while typing into it: https://cockpit-logs.us-east-1.linodeobjects.com/pull-1872-0fae78ec-20241024-124831-rhel-9-6/log.html#79

@garrett
Copy link
Member

garrett commented Oct 24, 2024

  1. The to-the-side thing looks too noisy with the icons.
  2. Can we make the icons the same color as the text? They're meant to call attention, but not that much attention.
  3. Perhaps we could use the clock icon (fa-clock) for old releases too. Having an icon there does help differentiate the footer text from the group text, and having these two icon types helps to express what each one is at a glance (without having to read it fully).
  4. Thanks for implementing these suggestions!

Quick screenshot mod to show the icons all the same color as the text (roughly). If PF doesn't let us do that, setting color: inherit might work, although it would probably need fill: currentcolor instead (as it's SVG).

image

With this version (below) and these mods (grey icons, and icons of some sort for all with the old/expired text), I'd consider it pretty much ready from a design standpoint, provided the UX works as expected.

@mvollmer
Copy link
Member Author

With this version (below) and these mods (grey icons, and icons of some sort for all with the old/expired text), I'd consider it pretty much ready from a design standpoint, provided the UX works as expected.

Tada:

image

@garrett
Copy link
Member

garrett commented Oct 24, 2024

It looks great overall! Thanks for picking this up and adapting to the requested changes!

However, I see a pretty big issue: The menu extends off the page and isn't scrollable.

Screenshot:

image

It needs a max height and it needs to be scrollable. I thought PF provided this being set already? (If not, there's a way to achieve this with some CSS, but I don't think it should be necessary?)

You could do something like this:

.pf-v5-c-select__menu {
  max-block-size: 50vh;
  overflow: auto;
}

(It should not be that exact CSS, as it would need be tied to that specific component, as you don't want to affect all pf-v5-c-select__menus on the page.)

But, again, I'm pretty sure this should be handled by PF already.

@mvollmer
Copy link
Member Author

However, I see a pretty big issue: The menu extends off the page and isn't scrollable.

Yes, this is fixed by cockpit-project/cockpit#21156. This PR needs to be rebased to pick it up.

Also, the recent port to TypeaheadSelect solves this with an addition to machines.sccs.

@mvollmer
Copy link
Member Author

But, again, I'm pretty sure this should be handled by PF already.

We have a fix for this in pkg/lib/patternfly/patternfly-5-overrides.scss. Without that, dropdowns don't seem to scroll ever.

@mvollmer
Copy link
Member Author

I have ported this over to the TypeaheadSelect template. It doesn't have grouping, but I was able to fake some headers, which is all we want.

image

image

@mvollmer mvollmer force-pushed the show-all-oses-for-download branch 2 times, most recently from 851b039 to 8b9c8c0 Compare October 25, 2024 11:36
@mvollmer
Copy link
Member Author

mvollmer commented Oct 25, 2024

So, the TypeaheadSelect template uses a different style for the "clear" button. This clashes now with our Autocomplete widget, which uses the deprecated Select. Compare the "Installation source" and "Operating system" fields:

image

Also, there are some unexpected pixel changes... Some dropdown arrows change color slightly, and some lines change their shadow...

@mvollmer mvollmer force-pushed the show-all-oses-for-download branch 2 times, most recently from befdfe9 to 32dbe58 Compare October 25, 2024 13:54
@mvollmer
Copy link
Member Author

mvollmer commented Oct 25, 2024

So, the TypeaheadSelect template uses a different style for the "clear" button. This clashes now with our Autocomplete widget, which uses the deprecated Select. Compare the "Installation source" and "Operating system" fields:

I have addressed that by copying the template into our project and adjusting it slightly to look like the deprecated Select.

Alternatively, I could try to pull the same "headers instead of groups" trick with the deprecated Select. It's doable, I guess, but... I think it's better to move forward here incrementally. Next step would be to stop using the deprecated Select in the FileAutocomplete component, and then replace the rest with the SimpleSelect template.

PatternFly 6 looks very similar to PF 5, and it has the same templates. So the work outlined above will likely carry over to PF 6 just fine.

@garrett
Copy link
Member

garrett commented Oct 28, 2024

PatternFly 6 looks very similar to PF 5, and it has the same templates. So the work outlined above will likely carry over to PF 6 just fine.

PatternFly 6 looks extremely different from PatternFly 5... unless you're talking about the code here?

So, the TypeaheadSelect template uses a different style for the "clear" button. This clashes now with our Autocomplete widget, which uses the deprecated Select. Compare the "Installation source" and "Operating system" fields:

Where is that provided, in our own code or in PF itself? They should definitely be consistent.

Oddly, PF icons say to use the X without the circle, and has a circleless X to clear @ https://www.patternfly.org/design-foundations/icons/#all-icons:

image

And, at least for now, it's the same in the upcoming PF6:

image

PF6's x icon in the dropdown is also a "naked" x, not a circle one:

image
(Press F for Florida)

But considering the description text is repeated several times, I'd say we can't depend on the website to be perfectly accurate yet. (PF6 is beta after all, and I'm sure they're fixing the website still too.)


So, in other words, I think we need to change the (x) to x without a circle. Both for PF5 and also for PF6.

@mvollmer
Copy link
Member Author

PatternFly 6 looks very similar to PF 5, and it has the same templates. So the work outlined above will likely carry over to PF 6 just fine.

PatternFly 6 looks extremely different from PatternFly 5... unless you're talking about the code here?

Yes, just the code. I was surprised by the stylistic differences, tbh.

@mvollmer
Copy link
Member Author

Let's reboot this, and put it on top of the TypeaheadSelect work in cockpit itself.

This will show all operating systems, regardless of EOL or release
dates, but the recommended ones will be at the top.  Operating systems
with an expired EOL date will be annotated accordingly in the list.

The tests have been brought up-to-date with regard to what OSes are
actually in the list and are downloadable.

Fixes cockpit-project#509
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow inclusion of EOL OSes in list
3 participants