lib: Port us away from simple deprecated Select variant #21356

mvollmer · 2024-11-29T09:26:25Z

Pixel differences: https://github.com/cockpit-project/pixel-test-reference/compare/1f17d8f6d2b39be2c9f19771004fe27c52c1bbce..baa39edeb94890093b3af6bc2e6802279e6d60eb

I think all count as bug fixes.

pkg/lib/cockpit-components-firewalld-request.jsx

mvollmer · 2024-12-04T08:31:12Z

@garrett, how should normal dropdown selects look?

Like this (a):

Or like this (b):

FormSelect and the deprecated Select that we are still using in many places look like (a). The new simple Select introduced in this PR would look like (b) because of our fix for patternfly/patternfly#6632 in patternfly-5-overrides.scss. With upstream PF5 styling, the new simple Selects would also look like (a).

Since we have traditionally been okay with how FormSelect and the deprecated Select look, I assume for now that the new simple Select should continue to look like them, i.e. like (a).

Consequently, I have narrowed the scope of the "double spacing" bug fix to the dropdowns in the Shell masthead. I think that's the only place where we want it (assuming (a) is the desired look for select dropdowns). Without the bug fix, the masthead dropdowns look uncentered since the large icon on their left doesn't have a margin, while the dropdown icon on the right has one.

mvollmer · 2024-12-04T09:40:52Z

The pixel tests and git grep found 2 dropdowns that are affected by our current "double spacing" bug fix: zoom controls and the "Add member" menu of bonds/bridges/etc.

Consequently, they currently look like (b) above, and thus look different from other dropdowns. This PR will change them to look like (a) as a side effect.

mvollmer · 2024-12-04T09:43:27Z

If we decide that dropdown should look like (b) everywhere (which I wouldn't object to at all), then we should do that in one step and change the looks of the deprecated Select and FormSelect in a separate PR and block this one on it.

mvollmer · 2024-12-04T10:59:40Z

@garrett, now about the background color... The non-deprecated MenuToggle that this PR uses for simple Select dropdown has a transparent background in light mode, while the deprecated Select has regular white. This makes a difference when the page background is not also white, like in toolbars:

With deprecated Select:

With non-deprecated MenuToggle:

Note that the "Filters" SearchInput input group has a white background but the "Pause" button is transparent. As far as I can tell, this is the default PF behavior.

Which should it be?

As with the icon margins, I have made it so that this PR keeps the current look.

garrett · 2024-12-05T08:48:42Z

b is definitely incorrect; a looks more correct at a glance

the widgets should all have a white background there, including the secondary button; they need contrast... the text on grey isn't enough contrast, and additionally the grey on the widgets make them look disabled

martinpitt

Thanks! The only actual bug here (I think) is the accidentally dropped isDisabled; plus two questions.

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

martinpitt · 2024-12-05T08:50:42Z

pkg/systemd/logs.jsx

+const timeFilterParams = {
+    current: { param: "boot", value: 0 },


Could we somehow allow "any additional junk" in the type declaration of MenuItem, so that we can continue to add these extra fields right in their declaration? and they are passed through event handlers opaquely?

Hmm, interesting. I had assumed that SelectOption.value needs to be a string (or number) because that's what the templates use. But it can in fact be "any" value.

The templates use value as a key, and this wont work when value can be any value.

So...I would have to go "back to the drawing board". Have to think about it.

But, I would agree that the changes here in logs.jsx are too large. I wanted to make the use of SimpleSelect trivial, but that has caused risky and confusing changes to the rest of the code. I'll roll that back a bit probably.

What about allowing

{ content: _("Current boot"), value: { key: "boot", value: 0 }, }

and we use content as the React key?

FTR, I'm completely happy with the values here. Restricting them to strings (or perhaps also numbers) keeps things easy and non-ambiguous. I was just asking if we could merge timeFilterParams into timeFilterOptions by just adding extra param and systemd_value (or similar) keys.

Both value and content are fine for the key, but I think value is a bit cleaner (i.e. the status quo)

In other words, can we declare SimpleSelectMenuOption as content: string, value: string | number, plus "any additional stuff" like [string]: unknown?

If that's difficult/impractical then ignore me. I was just wondering if object types can be partially specified and partially opaque.

Done. content can be any React.ReactNode so it can't always be used as a key, so we fall back to the ... index. :-/ But a explicit key in SimpleSelectOption is also supported.

In other words, can we declare SimpleSelectMenuOption as content: string, value: string | number, plus "any additional stuff" like [string]: unknown?

Hmm, we could, but I think it's not a good idea. It already has all the options from SelectOption, which inherits from MenuItem, and I think the risk of collision is just too big. For example, both key and value are already taken.

The old code also didn't do this. It didn't have

<SelectOption key='boot' value='-1'>Previous boot</SelectOption>

in it, but constructed it from timeFilterOptions.

I see. I clearly don't understand this enough, and it looks fine - I was just curious. So let's just keep this. Thanks!

pkg/lib/serverTime.js

mvollmer · 2024-12-05T13:56:24Z

b is definitely incorrect; a looks more correct at a glance

Ok, thanks! Then this PR is already correct.

the widgets should all have a white background there, including the secondary button; they need contrast... the text on grey isn't enough contrast, and additionally the grey on the widgets make them look disabled

Right, the this PR is also correct re the background.

I'll fix the button in a separate PR.

Thanks!

martinpitt

Thanks! One more raised eyebrow.

martinpitt · 2024-12-05T15:11:51Z

pkg/systemd/logs.jsx

+    { value: { key: "boot", value: 0 }, content: _("Current boot") },
+    { value: { key: "boot", value: "-1" }, content: _("Previous boot") },
+    { value: { key: "since", value: "-24hours", default: true }, content: _("Last 24 hours") },
+    { value: { key: "since", value: "-7days" }, content: _("Last 7 days") },


How are these not duplicate React keys?

I know the old code had this as well, and maybe that's just misleading -- these may not actually be React keys, but a custom value that we use to construct the journalctl argv? But then, the value is being used as the React key And that should be a string, not an object. How does this work?

these may not actually be React keys

Correct. The `key`` field of the value object has nothing to do with React. It is used in the URL.

The React key is now content. Or you can specify it explicitly as well, like

{ key: 12, value: { key: "boot", value: 0 }, content: _("Current boot") }

(There was an intermediate version of this PR that had renamed key to param. But I reverted it once it become no longer necessary to reduce changes to the code.)

I changed this back to use value as the key and document the need for an explicit key when value isn't suitable.

From 3b857f8b06440bccc8e7979cf686f4e3261ff4df

But not yet for the "checkbox" variant.

So that the SimpleSelect template looks like the deprecated Select that it is replacing, and like the FormSelect that we are also using. Instead, fix the dropdown margings in the Shell masthead, to keep them as they were.

The default is transparent, which would make them look grey in a grey toolbar for example. The old deprecated Select had a white background in those contexts, so let's emulate it.

The linter complains otherwise, since we no longer use it in Cockpit itself. There are no other external users either, afaics.

…rity OSesWithoutTracer is a list of globs.

martinpitt · 2024-12-10T14:30:57Z

This pixel change feels like a regression to me, but if that's hard to fix, I don't consider it a blocker. This pixel change is a bug fix and can be accepted into the reference.

martinpitt

Thanks! As I wrote above, the pixel refs need to be updated. I'm ok with that spacing regression becoming a follow-up, unless you already know how to fix it and it's easy.

Otherwise, code looks nice to me!

martinpitt · 2024-12-10T14:35:19Z

pkg/systemd/logs.jsx

+    { key: 1, value: { key: "boot", value: 0 }, content: _("Current boot") },
+    { key: 2, value: { key: "boot", value: "-1" }, content: _("Previous boot") },
+    { key: 3, value: { key: "since", value: "-24hours", default: true }, content: _("Last 24 hours") },
+    { key: 4, value: { key: "since", value: "-7days" }, content: _("Last 7 days") },


Thanks, that avois the confusion around what key is. (A bit weird to use numbers which don't mean anything, but that's fine).

martinpitt

Thanks! You removed no-test after force-pushing, so nothing ran yet. I triggered the tests.

cockpituous · 2024-12-11T01:13:18Z

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

+  });
+
+  const onToggleClick = () => {
+    onToggle && onToggle(!isOpen);


This added line is not executed by any test.

cockpituous · 2024-12-11T01:13:18Z

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

+  const _onSelect = (_event: React.MouseEvent<Element, MouseEvent> | undefined, value: unknown) => {
+    if (value)
+      onSelect(value);
+    onToggle && onToggle(true);


This added line is not executed by any test.

cockpituous · 2024-12-11T01:13:18Z

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

+
+  let content: React.ReactNode = placeholder;
+  if (toggleContent)
+    content = toggleContent;


This added line is not executed by any test.

cockpituous · 2024-12-11T01:13:18Z

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

+      onOpenChange={(isOpen) => {
+        onToggle && onToggle(isOpen);
+        setIsOpen(isOpen);


These 3 added lines are not executed by any test.

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

mvollmer force-pushed the lib-simple-select branch from 4658e3e to 93eaba8 Compare November 29, 2024 12:09

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

View reviewed changes

pkg/lib/cockpit-components-firewalld-request.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the lib-simple-select branch from 93eaba8 to c19dbe7 Compare November 29, 2024 12:25

mvollmer mentioned this pull request Dec 2, 2024

lib: Port us away from deprecated Select variant "checkbox" #21373

Merged

mvollmer force-pushed the lib-simple-select branch 2 times, most recently from 5f566fc to 424c36c Compare December 3, 2024 10:20

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

mvollmer marked this pull request as ready for review December 3, 2024 10:46

mvollmer force-pushed the lib-simple-select branch 2 times, most recently from e32a887 to 989b764 Compare December 3, 2024 13:10

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

mvollmer force-pushed the lib-simple-select branch from 989b764 to 0b9fea2 Compare December 4, 2024 08:35

mvollmer force-pushed the lib-simple-select branch from 0b9fea2 to 93dc76c Compare December 4, 2024 10:45

mvollmer force-pushed the lib-simple-select branch from 93dc76c to 5acb60f Compare December 4, 2024 12:17

mvollmer requested a review from garrett December 4, 2024 12:28

mvollmer force-pushed the lib-simple-select branch from 5acb60f to 8760214 Compare December 4, 2024 12:51

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

mvollmer force-pushed the lib-simple-select branch from 8760214 to dd310b3 Compare December 4, 2024 14:25

mvollmer requested a review from jelly December 4, 2024 14:25

mvollmer force-pushed the lib-simple-select branch from eff00ed to 5aa781c Compare December 4, 2024 15:39

mvollmer requested a review from martinpitt December 5, 2024 06:40

martinpitt requested changes Dec 5, 2024

View reviewed changes

mvollmer force-pushed the lib-simple-select branch from 5aa781c to f15aa5d Compare December 5, 2024 13:49

mvollmer force-pushed the lib-simple-select branch from f15aa5d to 2708fa2 Compare December 5, 2024 14:14

martinpitt requested changes Dec 5, 2024

View reviewed changes

mvollmer added 12 commits December 10, 2024 14:26

lib: Import SimpleSelect template from PatternFly

d7293c0

From 3b857f8b06440bccc8e7979cf686f4e3261ff4df

lib: Correct TypeScript errors in SimpleSelect

5ae7ece

lib: Make our changes to SimpleSelect

a04479c

logs: Use SimpleSelect instead of deprecated Select

e0cd2cf

metrics: Use SimpleSelect instead of deprecated Select

c5532d9

But not yet for the "checkbox" variant.

lib: Use SimpleSelect for ServerTime

b9918c0

lib: Use SimpleSelect for ShutdownModal

2f37d04

lib: Use SimpleSelect for FirewalldRequest

b9303f8

lib: Don't apply double spacing fix globally to all dropdowns

c55d1f9

So that the SimpleSelect template looks like the deprecated Select that it is replacing, and like the FormSelect that we are also using. Instead, fix the dropdown margings in the Shell masthead, to keep them as they were.

lib: Explicitly use white background color for MenuToggle

770bc7d

The default is transparent, which would make them look grey in a grey toolbar for example. The old deprecated Select had a white background in those contexts, so let's emulate it.

test: Remove Browser.select_PF_deprecated

a836426

The linter complains otherwise, since we no longer use it in Cockpit itself. There are no other external users either, afaics.

test: Correctly check for tracer presence in TestUpdates.testInfoSecu…

07bf0df

…rity OSesWithoutTracer is a list of globs.

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

mvollmer force-pushed the lib-simple-select branch from 2708fa2 to 07bf0df Compare December 10, 2024 12:27

mvollmer requested a review from martinpitt December 10, 2024 13:01

martinpitt previously approved these changes Dec 10, 2024

View reviewed changes

mvollmer dismissed martinpitt’s stale review via db174a3 December 10, 2024 14:58

test: Update pixel test references

c948cea

mvollmer force-pushed the lib-simple-select branch from db174a3 to c948cea Compare December 10, 2024 14:58

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

martinpitt approved these changes Dec 11, 2024

View reviewed changes

martinpitt mentioned this pull request Dec 11, 2024

test: Add tests for MultiTypeaheadSelect #21417

Merged

cockpituous reviewed Dec 11, 2024

View reviewed changes

martinpitt merged commit 2d6af15 into cockpit-project:main Dec 11, 2024
86 checks passed

martinpitt mentioned this pull request Dec 12, 2024

Makefile: Update Cockpit lib to 35090b743b6b4536fc9c09cd9644c1c3 cockpit-project/cockpit-files#875

Merged

mvollmer mentioned this pull request Dec 13, 2024

Makefile: Update Cockpit lib to 35090b743b6b4536fc9c09cd9644c1c3 cockpit-project/cockpit-machines#1947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Port us away from simple deprecated Select variant #21356

lib: Port us away from simple deprecated Select variant #21356

mvollmer commented Nov 29, 2024 •

edited

Loading

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

garrett commented Dec 5, 2024

martinpitt left a comment

martinpitt Dec 5, 2024

mvollmer Dec 5, 2024

mvollmer Dec 5, 2024

martinpitt Dec 5, 2024

martinpitt Dec 5, 2024

mvollmer Dec 5, 2024

mvollmer Dec 5, 2024

martinpitt Dec 5, 2024

mvollmer commented Dec 5, 2024

martinpitt left a comment

martinpitt Dec 5, 2024

mvollmer Dec 10, 2024

mvollmer Dec 10, 2024 •

edited

Loading

martinpitt commented Dec 10, 2024

martinpitt left a comment

martinpitt Dec 10, 2024

martinpitt left a comment

cockpituous Dec 11, 2024

cockpituous Dec 11, 2024

cockpituous Dec 11, 2024

cockpituous Dec 11, 2024

		const timeFilterParams = {
		current: { param: "boot", value: 0 },

lib: Port us away from simple deprecated Select variant #21356

lib: Port us away from simple deprecated Select variant #21356

Conversation

mvollmer commented Nov 29, 2024 • edited Loading

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

mvollmer commented Dec 4, 2024

garrett commented Dec 5, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Dec 5, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

martinpitt commented Dec 10, 2024

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer commented Nov 29, 2024 •

edited

Loading

mvollmer Dec 10, 2024 •

edited

Loading