lib: Support headers with TypeaheadSelect #21183

mvollmer · 2024-10-29T10:01:44Z

lib: Introduce our own TypeaheadSelect and use it everywhere #21175
tests for the new TypeaheadSelect in general
Allow dividers before headers and treat them right during filtering

test/verify/check-lib-typeahead

test/verify/check-lib

mvollmer · 2024-11-12T13:38:12Z

@garrett, I need your design input for how the headers should look.

PF5 headers look like this:

garrett · 2024-11-13T16:37:49Z

@garrett, I need your design input for how the headers should look.

PF5 headers look like this:

Yes, that's how PF5 headers look.

What are you looking for specifically? I'm not sure what you're asking.
How would I check this out to verify that it looks like PF5? (Like, which page, modal, widget?) I've poked around after building the branch and it's not clear what I should be looking at.

garrett · 2024-11-18T10:10:05Z

I'm looking ahead to PF6 to see what they now think headings (not headers) look like... They have some issues in PF6. 🤨

So I think we'll probably need custom stuff when we upgrade to PF6 anyway.

I did find this in the design for the non-form selects, but it's from PF5:

From https://www.patternfly.org/components/menus/select/design-guidelines#select-lists-with-favorites

But then they also have grouped items and I didn't even notice that it had headings, until I hovered over it...

https://www.patternfly.org/components/menus/select#with-grouped-items

But they have other style problems too, like super-low contrast for disabled items and not found messages:

garrett · 2024-11-18T10:22:36Z

After looking at everything in both PF5 and PF6:

Headings should be styled differently from entry items. (PF6 is technically using a slightly different color, but it's not obvious at all. The font size and/or weight needs to change too; otherwise it's just too subtle.)
The light color for disabled and error messages in PF6 is not AA contrast compliant, and probably cant even be read on cheaper displays or in bright environments. Even under ideal conditions, many people won't be able to read it, and it can even cause eyestrain to those who can.
All the examples I've found from PF5 and PF6 use separators between groups, in addition to the headings. Headings alone are not enough, especially if there is description text.

Since we want to use headings and description text in #1872, I think we need dividers between the sections on by default. (Perhaps with a way to turn them off if we don't have description text.)

garrett · 2024-11-18T10:28:20Z

OK, so TL;DR from the above, focusing on what we need to do:

For now, in this PR:

Specifically thinking of create: Show all operating systems cockpit-machines#1872, make sure there are dividers between sections, probably on by default. They should definitely be present when there are descriptions. (It could alternative be automatically on if descriptions exist. Or just always on.)

For the future, outside of this PR:

When we upgrade to PF6, I think we'll either need to customize things in our widget and/or have some CSS overrides to fix their issues.
We need to file some upstream issues with PF6 problems. (Contrast problems, spacing problems, size issues as the headers are currently indistinguishable, etc.) I've communicated all this before a few times with PF6 during the pre-alpha and alpha phases, but now that it's released and still has many of the the same problems I reported multiple times, I need to file specific bugs to have them fixed and we'll probably need local overrides still, as it'll be unclear as to when they'll fix these actual quantifiable UI, UX, and accessibility problems.

mvollmer · 2024-11-19T08:11:56Z

make sure there are dividers between sections, probably on by default.

Roger!

martinpitt

Thanks! A grab-bag of questions and minor ignorable stuff, but I think at least the dark mode should be fixed.

pkg/lib/cockpit-components-typeahead-select.tsx

pkg/playground/react-demo-typeahead.jsx

test/verify/check-lib

mvollmer · 2024-11-22T09:55:27Z

Thanks! A grab-bag of questions and minor ignorable stuff, but I think at least the dark mode should be fixed.

Thanks for the review! This is indeed from my very early days of TypeScript exposure, and I really should go back and make it more solid. For example, I'll try to encode in the types that dividers and headers are different animals than regular items. They don't have isDisabled, for example, and will need a key property.

martinpitt

👏 Thanks!

martinpitt · 2024-11-22T15:08:56Z

pkg/lib/cockpit-components-typeahead-select.tsx

+export interface TypeaheadSelectDividerOption {
+  decorator: "divider";


This is really cool!

martinpitt · 2024-11-22T15:11:06Z

pkg/playground/react-demo-typeahead.tsx

+import { Checkbox } from '@patternfly/react-core';
+import { TypeaheadSelect, TypeaheadSelectOption } from "cockpit-components-typeahead-select";
+
+const TypeaheadDemo = ({ options } : { options: TypeaheadSelectOption[] }) => {


I'm really amazed that you could fully type this without too much effort. Makes the "type ahead demo" twice as interesting! 😉

martinpitt · 2024-11-22T15:13:10Z

test/verify/check-lib

+
+
+@testlib.nondestructive
+class TestLib(testlib.MachineCase):


Hmm, the others are in check-pages, but not that important.

Yeah, I thought I start a new pattern for cockpit-component tests. I was always surprised to find them in check-pages.

mvollmer · 2024-11-26T07:21:06Z

pkg/lib/cockpit-components-typeahead-select.tsx

  ) => {
    onSelect && onSelect(_event, option.value);
    closeMenu();
  };

  const _onSelect = (_event: React.MouseEvent<Element, MouseEvent> | undefined, value: string | number | undefined) => {
    if (value && value !== NO_RESULTS) {
-      const optionToSelect = selectOptions.find((option) => option.value === value);
+      const optionToSelect = selectOptions.find((option) => isMenu(option) && option.value === value) as TypeaheadSelectMenuOption | undefined;


This could be better written as guard function.

This also changes the way dividers are encoded, so that TypeScript can better understand the option variants.

mvollmer mentioned this pull request Oct 29, 2024

create: Show all operating systems cockpit-project/cockpit-machines#1872

Draft

4 tasks

mvollmer force-pushed the lib-typeahead-select-groups branch 2 times, most recently from 866b34b to 5f5a440 Compare October 29, 2024 12:15

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 31, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch 3 times, most recently from a94ee95 to 018531b Compare October 31, 2024 13:15

mvollmer force-pushed the lib-typeahead-select-groups branch from 018531b to 5b6e088 Compare November 11, 2024 14:41

github-advanced-security bot found potential problems Nov 11, 2024

View reviewed changes

mvollmer force-pushed the lib-typeahead-select-groups branch 3 times, most recently from ccbfbe7 to ca76a3f Compare November 12, 2024 10:13

github-advanced-security bot found potential problems Nov 12, 2024

View reviewed changes

test/verify/check-lib Fixed Show fixed Hide fixed

mvollmer force-pushed the lib-typeahead-select-groups branch 2 times, most recently from 0ba70a1 to 79104d7 Compare November 12, 2024 12:27

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 12, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch from 79104d7 to 16d2e4f Compare November 12, 2024 13:35

mvollmer requested a review from garrett November 12, 2024 13:35

mvollmer marked this pull request as ready for review November 12, 2024 13:40

mvollmer force-pushed the lib-typeahead-select-groups branch 5 times, most recently from edd2893 to eccba0c Compare November 13, 2024 09:01

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 13, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch from eccba0c to 308138d Compare November 13, 2024 10:28

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 13, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch 3 times, most recently from 360522a to a00705d Compare November 19, 2024 14:13

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 19, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch from a00705d to 56960e1 Compare November 19, 2024 14:15

mvollmer requested a review from martinpitt November 21, 2024 14:56

martinpitt requested changes Nov 22, 2024

View reviewed changes

mvollmer added 3 commits November 22, 2024 14:39

playground: Enable dark mode for React Patterns

a2536a3

lib: Translate one more UI string in TypeaheadSelect

871efa5

lib: Ignore dividers when key navigating in TypeaheadSelect

4eff7b6

mvollmer force-pushed the lib-typeahead-select-groups branch from 56960e1 to 98b5897 Compare November 22, 2024 12:47

mvollmer requested a review from martinpitt November 22, 2024 12:51

mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 22, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch 2 times, most recently from 67bf149 to 0724681 Compare November 22, 2024 14:57

mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 22, 2024

mvollmer force-pushed the lib-typeahead-select-groups branch from 0724681 to 44ea1e1 Compare November 22, 2024 15:03

martinpitt previously approved these changes Nov 22, 2024

View reviewed changes

mvollmer commented Nov 26, 2024

View reviewed changes

lib: Support headers with TypeaheadSelect

68ac285

This also changes the way dividers are encoded, so that TypeScript can better understand the option variants.

mvollmer dismissed martinpitt’s stale review via 68ac285 November 26, 2024 07:30

mvollmer force-pushed the lib-typeahead-select-groups branch from 44ea1e1 to 68ac285 Compare November 26, 2024 07:30

mvollmer requested a review from martinpitt November 26, 2024 07:31

martinpitt approved these changes Nov 26, 2024

View reviewed changes

mvollmer merged commit 8d9f97b into cockpit-project:main Nov 26, 2024
83 of 85 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Support headers with TypeaheadSelect #21183

lib: Support headers with TypeaheadSelect #21183

mvollmer commented Oct 29, 2024 •

edited

Loading

mvollmer commented Nov 12, 2024

garrett commented Nov 13, 2024

garrett commented Nov 18, 2024

garrett commented Nov 18, 2024 •

edited

Loading

garrett commented Nov 18, 2024

mvollmer commented Nov 19, 2024

martinpitt left a comment

mvollmer commented Nov 22, 2024

martinpitt left a comment

martinpitt Nov 22, 2024

martinpitt Nov 22, 2024

martinpitt Nov 22, 2024

mvollmer Nov 26, 2024

mvollmer Nov 26, 2024

mvollmer Nov 26, 2024

		export interface TypeaheadSelectDividerOption {
		decorator: "divider";



		@testlib.nondestructive
		class TestLib(testlib.MachineCase):

lib: Support headers with TypeaheadSelect #21183

lib: Support headers with TypeaheadSelect #21183

Conversation

mvollmer commented Oct 29, 2024 • edited Loading

mvollmer commented Nov 12, 2024

garrett commented Nov 13, 2024

garrett commented Nov 18, 2024

garrett commented Nov 18, 2024 • edited Loading

garrett commented Nov 18, 2024

mvollmer commented Nov 19, 2024

martinpitt left a comment

Choose a reason for hiding this comment

mvollmer commented Nov 22, 2024

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Nov 22, 2024

Choose a reason for hiding this comment

martinpitt Nov 22, 2024

Choose a reason for hiding this comment

martinpitt Nov 22, 2024

Choose a reason for hiding this comment

mvollmer Nov 26, 2024

Choose a reason for hiding this comment

mvollmer Nov 26, 2024

Choose a reason for hiding this comment

mvollmer Nov 26, 2024

Choose a reason for hiding this comment

mvollmer commented Oct 29, 2024 •

edited

Loading

garrett commented Nov 18, 2024 •

edited

Loading