Allow EmptyStatePanel actions to be of a different variant #20763

jelly · 2024-07-15T14:31:17Z

This changes the EmptyStatePanel action to a link by default, retaining the primary button where the action is almost always guaranteed to be clicked (starting networkmanager for example).

Adds an action to the user/groups emptystate:

Makes the logs load earlier data show a spinner after clicking:

Screencast.from.2024-07-15.16-30-12.webm

And further cleans up API usage, and a small typing improvement.

Empty state now shows a link

No logs found now shows:

martinpitt

Thanks, nice cleanups! No strong opinion about the changed button type default, but is it worth silently breaking/changing all our projects for this?

(Also, still several test failures)

martinpitt · 2024-07-15T15:44:01Z

pkg/lib/cockpit-components-empty-state.jsx

@@ -24,7 +24,7 @@ import { EmptyStateActions, EmptyState, EmptyStateBody, EmptyStateFooter, EmptyS
 import { Spinner } from "@patternfly/react-core/dist/esm/components/Spinner/index.js";
 import "./cockpit-components-empty-state.css";

-export const EmptyStatePanel = ({ title, paragraph, loading, icon, action, isActionInProgress = false, onAction, secondary, headingLevel = "h1" }) => {
+export const EmptyStatePanel = ({ title, paragraph, loading, icon, action, isActionInProgress = false, onAction, actionVariant = "link", secondary, headingLevel = "h1" }) => {


Is the "most of the time" based on actual counting or gut feeling? We use this a lot, also in podman/machines, and I feel the other way around. I.e. keep the "primary" default, and make it link or secondary etc. in the new places where we need it?

This is indeed a backwards incompatible change, this whole panel is abused or used wrong :)

<EmptyStatePanel title={ cockpit.format(_("VM $0 does not exist on $1 connection"), cockpit.location.options.name, cockpit.location.options.connection) } action={ <Button variant="link" onClick={() => cockpit.location.go(["vms"])}> {_("Go to VMs list")} </Button> }

So Garrett's two cents where, most of the time you want a link (primary should be the exception), but I am also fine to make this non-breaking, that is better imo.

martinpitt · 2024-07-15T15:45:10Z

pkg/networkmanager/networkmanager.jsx

-                                     action={ name ? _("Start service") : null }
+                                     action={_("Start service")}


This was clearly meant to be nmService.name -- if it doesn't exist, we can't start it.

pkg/systemd/logsJournal.jsx

martinpitt · 2024-07-15T15:47:36Z

pkg/users/accounts-list.js

@@ -338,7 +338,7 @@ const GroupsList = ({ groups, accounts, isExpanded, setIsExpanded, min_gid, max_
                    rows={ filtered_groups.map(a => getGroupRow(a, accounts)) }
                    loading={ groups.length && accounts.length ? '' : _("Loading...") }
                    sortMethod={sortRows}
-                    emptyComponent={<EmptyStatePanel title={_("No matching results")} icon={SearchIcon} />}
+                    emptyComponent={<EmptyStatePanel title={_("No matching results")} icon={SearchIcon} action={_("Clear filter")} onAction={() => setCurrentTextFilter('')} />}


overly long line?

martinpitt

Thanks! Good cleanups!

pkg/systemd/logsJournal.jsx

For actions such as clearing the search filter, the action is not a primary one but optional for such actions it is expected to be a link.

The `name` variable is implicitly defined as `window.name` which is deprecated but also likely unintended, instead it should check if the networkmanager service exists.

The property actionInProgressText does not exist and never has for the EmptyStatePanel so drop it. When the user clicks on `Load earlier entries` show the EmptyStatePanel in loading state.

Loading is false is the default property value.

Allow users to clear the current search filter from the EmptyStatePanel.

garrett · 2024-07-17T09:51:20Z

Looks great overall!

Thanks for addressing this, @jelly

As discussed in Matrix, let's revert the button at the end of the list, as it gets a bit lost with so much text above it. (This is one of those cases it could go either way, but the deemphasis makes it less obvious here.)

Additionally, unrelated to the changes in this commit, we should do a follow-up fix for the headings here. When there's no content, there should also be no headings.

cockpituous · 2024-07-17T12:45:09Z

pkg/networkmanager/networkmanager.jsx

@@ -75,7 +75,7 @@ const App = () => {
                <div id="networking-nm-crashed">
                    <EmptyStatePanel icon={ ExclamationCircleIcon }
                                     title={ _("NetworkManager is not running") }
-                                     action={ name ? _("Start service") : null }
+                                     action={nmService.exists ? _("Start service") : null}


This added line is not executed by any test.

cockpituous · 2024-07-17T12:45:10Z

pkg/networkmanager/networkmanager.jsx

@@ -99,7 +99,7 @@ const App = () => {
                <div id="networking-nm-disabled">
                    <EmptyStatePanel icon={ ExclamationCircleIcon }
                                     title={ _("Network devices and graphs require NetworkManager") }
-                                     action={ name ? _("Enable service") : null }
+                                     action={nmService.exists ? _("Enable service") : null}


This added line is not executed by any test.

cockpituous · 2024-07-17T12:45:10Z

pkg/users/accounts-list.js

@@ -338,7 +338,8 @@ const GroupsList = ({ groups, accounts, isExpanded, setIsExpanded, min_gid, max_
                    rows={ filtered_groups.map(a => getGroupRow(a, accounts)) }
                    loading={ groups.length && accounts.length ? '' : _("Loading...") }
                    sortMethod={sortRows}
-                    emptyComponent={<EmptyStatePanel title={_("No matching results")} icon={SearchIcon} />}
+                    emptyComponent={<EmptyStatePanel title={_("No matching results")} icon={SearchIcon} action={_("Clear filter")}
+                                                     onAction={() => setCurrentTextFilter('')} actionVariant="link" />}


This added line is not executed by any test.

jelly requested a review from martinpitt July 15, 2024 14:31

martinpitt requested changes Jul 15, 2024

View reviewed changes

jelly force-pushed the empty-state-secondary branch from 0b6a69f to 17275e0 Compare July 16, 2024 09:48

jelly changed the title ~~Make EmptyStatePanel actions a link by default~~ Allow EmptyStatePanel actions to be of a different variant Jul 16, 2024

jelly force-pushed the empty-state-secondary branch 3 times, most recently from bb84c30 to 0101699 Compare July 16, 2024 15:37

jelly requested a review from martinpitt July 17, 2024 07:03

martinpitt previously approved these changes Jul 17, 2024

View reviewed changes

pkg/systemd/logsJournal.jsx Outdated Show resolved Hide resolved

jelly added 9 commits July 17, 2024 11:23

lib: allow configuring the action variant of an EmptyStatePanel

0ecb7d5

For actions such as clearing the search filter, the action is not a primary one but optional for such actions it is expected to be a link.

networkmanager: check if NetworkManager service exists

a0d6756

The `name` variable is implicitly defined as `window.name` which is deprecated but also likely unintended, instead it should check if the networkmanager service exists.

systemd: show loading state for EmptyStatePanel actions

74688c9

The property actionInProgressText does not exist and never has for the EmptyStatePanel so drop it. When the user clicks on `Load earlier entries` show the EmptyStatePanel in loading state.

systemd: drop default loading state for EmptyStatePanel

54e69cd

Loading is false is the default property value.

lib: type cockpit.logout

c84ec6a

metrics: use onActions EmptyStatePanel API

08159ef

systemd: use EmptyStatePanel's onAction API

0b034e2

lib: make loading explicitly default false in EmptyStatePanel

e583365

users: allow clearing the search filter in EmptyStatePanel

f2943bf

Allow users to clear the current search filter from the EmptyStatePanel.

jelly dismissed martinpitt’s stale review via f2943bf July 17, 2024 09:51

jelly force-pushed the empty-state-secondary branch from 0101699 to f2943bf Compare July 17, 2024 09:51

garrett mentioned this pull request Jul 17, 2024

Empty state tables should not show headings #20771

Open

martinpitt approved these changes Jul 17, 2024

View reviewed changes

cockpituous reviewed Jul 17, 2024

View reviewed changes

martinpitt merged commit 516c155 into cockpit-project:main Jul 17, 2024
75 of 77 checks passed

jelly deleted the empty-state-secondary branch July 17, 2024 13:09

martinpitt mentioned this pull request Jul 17, 2024

new Channel API #20731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow EmptyStatePanel actions to be of a different variant #20763

Allow EmptyStatePanel actions to be of a different variant #20763

jelly commented Jul 15, 2024 •

edited

Loading

martinpitt left a comment

martinpitt Jul 15, 2024

jelly Jul 16, 2024 •

edited

Loading

martinpitt Jul 15, 2024

martinpitt Jul 15, 2024

martinpitt left a comment

garrett commented Jul 17, 2024

cockpituous Jul 17, 2024

cockpituous Jul 17, 2024

cockpituous Jul 17, 2024

		action={ name ? _("Start service") : null }
		action={_("Start service")}

Allow EmptyStatePanel actions to be of a different variant #20763

Allow EmptyStatePanel actions to be of a different variant #20763

Conversation

jelly commented Jul 15, 2024 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Jul 15, 2024

Choose a reason for hiding this comment

jelly Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

martinpitt Jul 15, 2024

Choose a reason for hiding this comment

martinpitt Jul 15, 2024

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

garrett commented Jul 17, 2024

cockpituous Jul 17, 2024

Choose a reason for hiding this comment

cockpituous Jul 17, 2024

Choose a reason for hiding this comment

cockpituous Jul 17, 2024

Choose a reason for hiding this comment

jelly commented Jul 15, 2024 •

edited

Loading

jelly Jul 16, 2024 •

edited

Loading