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.

Sign up for GitHub

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

Jump to bottom

lib: Introduce our own TypeaheadSelect and use it everywhere #21175

Merged

mvollmer merged 12 commits into cockpit-project:main from mvollmer:lib-typeahead-select

Oct 31, 2024

Member

mvollmer commented Oct 28, 2024 •

edited

Loading

Keep isScrollable=false as default, and explicitly pass isScrollable=true everywhere. This matches the PF APIs better.
l10n for Message props

mvollmer added the no-test label

mvollmer force-pushed the lib-typeahead-select branch 4 times, most recently from c483dff to f59da49 Compare

October 28, 2024 12:35

github-advanced-security bot found potential problems

View reviewed changes

pkg/storaged/dialog.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the lib-typeahead-select branch from f59da49 to dc0e059 Compare

October 28, 2024 13:17

github-advanced-security bot found potential problems

View reviewed changes

pkg/storaged/dialog.jsx Fixed Show fixed Hide fixed

mvollmer force-pushed the lib-typeahead-select branch 6 times, most recently from cb132d7 to 2f4974d Compare

October 29, 2024 09:51

mvollmer mentioned this pull request

lib: Support headers with TypeaheadSelect #21183

Open

3 tasks

mvollmer removed the no-test label

mvollmer force-pushed the lib-typeahead-select branch 2 times, most recently from 48f1985 to 5e6acf0 Compare

October 29, 2024 11:51

mvollmer marked this pull request as ready for review

October 29, 2024 12:16

mvollmer marked this pull request as draft

October 29, 2024 12:16

mvollmer force-pushed the lib-typeahead-select branch 2 times, most recently from 3d62c7d to b116924 Compare

October 29, 2024 17:56

Member Author

mvollmer commented Oct 30, 2024

Only one pixel diff: https://cockpit-logs.us-east-1.linodeobjects.com/pull-21175-b116924b-20241029-201840-fedora-40-other/pixeldiff.html#TestJournal-testBasic-journal-filters-panel

Member Author

mvollmer commented Oct 30, 2024

I am happy with the huge coverage holes. shrug

mvollmer marked this pull request as ready for review

October 30, 2024 07:26

mvollmer requested review from garrett and martinpitt

October 30, 2024 07:26

martinpitt requested changes

View reviewed changes

Member

martinpitt left a comment

Thanks for the courage and work to address this long-outstanding porting!

I'm not thrilled by the code copy, but I agree it's a nice way to see what we need, and then spoon-feed it to upstream PRs. One hard blocker, and a bunch of nitpicks/questions.

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

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

Comment on lines 428 to 433

		{...(!(isFiltering && filterValue) && !selected ? { style: { display: 'none' } } : {})}
		{...(!(isFiltering && filterValue) && !(props.selected && onClearSelection) ? { style: { display: 'none' } } : {})}

Member

martinpitt Oct 30, 2024

Also feels like a straight upstreamable fix?

Member Author

mvollmer Oct 30, 2024

If it's a fix... I have to admit I am not super sure how the Clear button is supposed to behave... I'll submit it.

pkg/lib/cockpit-components-file-autocomplete.jsx Show resolved Hide resolved

test/verify/check-pages

Comment on lines -959 to +955

		b.set_file_autocomplete_val("#demo-upload", "/root/")
		b.set_file_autocomplete_val("#demo-upload", "/var/")

Member

martinpitt Oct 30, 2024

Why the path change?

Member Author

mvollmer Oct 30, 2024 •

edited

Loading

/root is not listable by admin, and our FileAutocomplete widget shows an error and doesn't let us select /root. Previously, it was possible to make any value stick as the selected value just by closing the dropdown (regardless of whether it exists, or an error occurred, even for !isCreatable). I consider that a bug, actually.

Now this is no longer possible and we need to use something that can be selected from the dropdown, but not be written to by admin. Also, we need to use something that is not a read-only filesystem on CoreOs.

So there are bugs here still to be fixed, I guess:

FileAutocomplete should probably not prevent the user from selecting a directory that can't be listed.
The upload machinery or the test here don't react correctly to a read-only filesystem, only to read-only permission bits.

test/verify/check-shell-keys Show resolved Hide resolved

mvollmer added 2 commits

October 30, 2024 14:54


          lib: Copy TypeaheadSelect into our code

2780a09

From 09b16829dcad32650412a66b7145ef8e7552c418.


          lib: Fix TypeScript error in TypeaheadSelect

f439627

mvollmer force-pushed the lib-typeahead-select branch 2 times, most recently from 0186e3b to 5a2b13d Compare

October 30, 2024 16:10

mvollmer added 10 commits

October 30, 2024 18:38


          lib: Optionally trust selected property in TypeaheadSelect

20918d8


          lib: Support dividers with TypeaheadSelect

f63307c


          lib: Hide clear button in TypeaheadSelect when it would do nothing

24e1888


          lib: Simplify and translate UI texts in TypeaheadSelect

6cf24cf


          networking: Use TypeaheadSelect instead of deprecated Select

b4dba20

And avoid duplicate entries.


          storage: Use TypeaheadSelect instead of deprecated Select

15ead09


          lib: Use TypeaheadSelect instead of deprecated Select for timezones

713897b


          systemd: Use TypeaheadSelect instead of deprecated Select

e170e9d


          lib: Use TypeaheadSelect instead of deprecated Select for FileAutoCom…

…plete


          test: Update pixel references for TypeaheadSelect changes

d425295

mvollmer force-pushed the lib-typeahead-select branch from 5a2b13d to d425295 Compare

October 30, 2024 16:41

martinpitt reviewed

View reviewed changes

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

+                 from gratuitous upstream changes.
+              */
+              /* eslint-disable */

Member

martinpitt Oct 30, 2024

Can this be restricted to whitespace errors, or is that too messy? Most other eslint complaints are rather useful

pkg/lib/cockpit-components-typeahead-select.tsx Outdated Show resolved Hide resolved

cockpituous reviewed

View reviewed changes

pkg/lib/cockpit-components-file-autocomplete.jsx

    
                                           onSelect={(_, value) => {

                                               this.setState({ value });

                                               this.onPathChange(value);

                                               this.props.onChange(value || '', null);

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

pkg/lib/cockpit-components-file-autocomplete.jsx

    
                                           }}

                                           onClearSelection={this.clearSelection}

                                           isCreatable={this.props.isOptionCreatable}

                                           createOptionMessage={val => cockpit.format(_("Create $0"), val)}

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

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

+                const NO_RESULTS = 'no results';
+                if (isCreatable && !createOptionMessage)
+                  throw "isCreatable requires createOptionMessage";

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

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

+                      !newSelectOptions.find((o) => String(o.content).toLowerCase() === filterValue.toLowerCase())
+                    ) {
+                      const createOption = {
+                        content: typeof createOptionMessage === 'string' ? createOptionMessage : createOptionMessage(filterValue),

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

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

+                        value: filterValue
+                      };
+                      newSelectOptions = isCreateOptionOnTop
+                        ? [createOption, ...newSelectOptions]

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

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

+                    isOpen={isOpen}
+                    selected={selected}
+                    onSelect={_onSelect}
+                    onOpenChange={(isOpen) => !isOpen && closeMenu()}

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

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

+                    <SelectList>
+                      {filteredSelections.map((option, index) => {
+                        if (option.divider)
+                            return <Divider key={option.key || index} component="li" />;

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

pkg/lib/serverTime.js

    
                                  <TypeaheadSelect toggleProps={ { id: "systime-timezones" } }

                                                   isScrollable

                                                   selected={time_zone}

                                                   onSelect={(event, value) => { change("time_zone", value) }}

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

pkg/storaged/dialog.jsx

+                                       createOptionMessage={val => cockpit.format(_("Use $0"), val)}
+                                       selected={value}
+                                       onSelect={(_, value) => change(value)}
+                                       onClearSelection={() => change("")}

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

pkg/systemd/logs.jsx

+                                       selected={identifiersFilter}
+                                       selectedIsTrusted
+                                       onSelect={(e, selection) => { onIdentifiersFilterChange(selection) }}
+                                       onClearSelection={identifiersFilter != "all" && (() => { onIdentifiersFilterChange("all") })}

Contributor

cockpituous Oct 30, 2024

This added line is not executed by any test.

martinpitt approved these changes

View reviewed changes

Member

martinpitt left a comment

Thanks for the updates! Frankly this looks great to me now, definitively an improvement over using the deprecated component.

#21175 (comment) still mentions some potential bug fixes, but I think these aren't regressions, but follow-ups?

Member Author

mvollmer commented Oct 31, 2024

#21175 (comment) still mentions some potential bug fixes, but I think these aren't regressions, but follow-ups?

Yes.

mvollmer merged commit e57b414 into cockpit-project:main

87 checks passed

mvollmer removed the request for review from garrett

October 31, 2024 07:47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet