Show dialog errors at the top #17221

martinpitt · 2022-04-05T09:57:13Z

See @garrett's review in #17214 (review) . This makes our dialogs comply with PatternFly form validation design.

On main they look like this:

With this PR they look like this:

Initial draft which just converts cockpit-components-dialog and a few <ModalError> instances.

martinpitt · 2022-04-05T09:59:43Z

@garrett : Is this the direction that you had in mind? I can't say I like it (it makes the dialog contents jump around), but PF makes this rather clear. So I'm mostly asking whether I am missing something in general, before I spend more time to fix the rest. They all need to be tested individually, e.g. one dialog didn't have a <Form> so the spacing was wrong. And the timer dialog had completely broken error reporting.

jelly · 2022-04-05T11:30:25Z

Just had this with kpatches, it is shown at the top but it's missing some spacing in my opinion.

Does this PR handle that too?

martinpitt · 2022-04-05T11:32:00Z

@jelly : Thanks for spotting -- I suppose that's another case of missing a <Form> wrapper. As this is an utterly complex endeavour, I'll split up the cockpit-components-dialog PR from the <ModalError> PR -- even individually they are hard enough.

jelly · 2022-04-05T11:39:10Z

@jelly : Thanks for spotting -- I suppose that's another case of missing a <Form> wrapper. As this is an utterly complex endeavour, I'll split up the cockpit-components-dialog PR from the <ModalError> PR -- even individually they are hard enough.

Yup, it's missing a <Form> shall I make it a part of the my install kpatches update branch?

This looks fine now

martinpitt · 2022-04-05T12:01:43Z

@jelly : Sure, why not -- then let's block this PR on your's as well.

garrett · 2022-04-06T08:36:43Z

@garrett : Is this the direction that you had in mind? I can't say I like it (it makes the dialog contents jump around), but PF makes this rather clear.

Yes. And I agree about not liking the contents jump around; I don't like that either and it's probably why it was on the bottom.

But, yes, PF is very clear about this.

And this is why we should, in most cases, have errors situated at the origin of error (as in: the widget that has problematic data, such as inputs with invalid entries) whenever possible.

However, in every single example above, there's no amount of changing the values that can affect the outcome. There's no permission or something going wrong when saving, so there's really no problem pushing down the form which cannot fix the problem anyway... at least in these examples.

So I'm mostly asking whether I am missing something in general, before I spend more time to fix the rest. They all need to be tested individually, e.g. one dialog didn't have a <Form> so the spacing was wrong. And the timer dialog had completely broken error reporting.

Yeah, thankfully we caught that. Forms really should have a <Form> of some type; the HTML specs are pretty clear about <form> being needed for widgets to be useful (at least without sprinkling in JavaScript to do something not necessarily related to a form). If it happened to work before, then that's just browsers trying to deal with invalid HTML and doing something that approximates what is expected.

If an element is not nested within in a form, then a form association can be attached. Otherwise, its form association is null (which was the case here):

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#association-of-controls-and-forms

(This is about browser implementations and fallbacks.)

Here's MDN specifically pointing out <input form="formID">: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-form

It's best to just nest within a <form> instead of using the form attribute to point to a <form>, as the attribute just makes things confusing and loses context of other form elements. It exists as a fallback mechanism:

The form attribute lets you place an input anywhere in the document but have it included with a form elsewhere in the document.

But then note:

Note: An input can only be associated with one form.

(But it should be associated with a form. Ideally nested within. But rare circumstances require the use of the form attribute instead.)

garrett · 2022-04-07T15:03:29Z

FWIW: We would ideally have this PR, #17221, and PRs similar to the machines one for modals in Cockpit, Cockpit-Podman, etc. all land in the same release.

jelly · 2022-04-08T16:00:00Z

Found out that cockpit-machines needs fixing:

garrett · 2022-04-21T15:26:49Z

Just checking in: What's the status of this PR? Is it waiting on anything or anyone? (It's still marked as draft, FWIW.)

The screenshots look good to me, so I'm happy with what I see so far. Triggering all the errors might be a bit difficult to see it aside from the screenshots, however.

martinpitt · 2022-04-21T16:43:03Z

@garrett : Not blocked, I just didn't find the time to look into this again. This is a huge piece of work, but splitting it up into several independent PRs runs the risk of being inconsistent in a release.

garrett · 2022-04-25T10:04:16Z

Not blocked

Whew. I was worried that I was blocking it. 😅 👍

This is a huge piece of work, but splitting it up into several independent PRs runs the risk of being inconsistent in a release.

Yep. This is definitely one of those massive PRs that touches a lot of the codebase and needs to be all-in-one.

garrett · 2022-11-07T15:28:13Z

Can we get this looked at again and merged soon, please? Thanks! ✨

KKoukiou · 2022-11-30T12:15:36Z

Updated all dialogs I could find with alerts in modal. All affected dialogs seen in the screenshots below.

martinpitt · 2022-11-30T12:35:37Z

@KKoukiou : Thank you! This may also affect some pixel tests, or don't we have any errors there?

KKoukiou · 2022-11-30T13:59:31Z

@KKoukiou : Thank you! This may also affect some pixel tests, or don't we have any errors there?

I will wait for the test results and see :)

KKoukiou

Self approving - but I would love someone else to take a look as well :)

martinpitt · 2022-12-01T13:02:53Z

Thanks @KKoukiou for finishing this! The first few changes in the pixel test comparison look like acceptable noise. However, the "Switch to administrative access" dialog got a lot of extra space now, which looks sad. Is this easy to fix?

garrett · 2022-12-01T15:41:34Z

Thanks for the comparison screenshots!

It's not the only one with extra space, for example:

And there's extra space here, too (also above "these changes"; the differentiating space below is due to the scroll region content being different):

(And there are a bunch of others, but I think most are variants.)

Additionally, there's a bug in the dark theme as seen here:

(The white in the table becomes black, whereas the white in the modal becomes dark grey.)

...but it's not a problem in this PR itself; it's just visible here. This one particular bug with the dark theme lists should be fixed elsewhere.

Nothing uses the footer outside of the Dialog class, and it's not an API that makes much sense on its own. Likewise, the Dialog class is very hard to use standalone without `show_modal_dialog()`, and nothing imports it. Make these private, so that we can change their implementation.

See https://www.patternfly.org/v4/components/form/design-guidelines/#error-validation-on-submission

martinpitt · 2022-12-02T08:47:23Z

Thanks @KKoukiou ! LGTM, but I can't approve as it's my PR

jelly · 2022-12-02T10:08:15Z

Thanks @KKoukiou ! LGTM, but I can't approve as it's my PR

Approved!

cockpituous · 2022-12-03T10:28:49Z

pkg/metrics/metrics.jsx

-                        isDisabled={ !dialogLoggerValue }
-                        onChange={enable => setDialogProxyValue(enable)} />
+            <Stack hasGutter>
+                { dialogError && <ModalError dialogError={ _("Failed to configure PCP") } dialogErrorDetail={dialogError} /> }


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:49Z

pkg/playground/react-patterns.js

@@ -71,10 +71,11 @@ const onDialogDone = function(success) {
    document.getElementById("demo-dialog-result").textContent = "Dialog closed: " + result + "(" + action + ")";
 };

-const onStandardDemoClicked = function(staticError) {
+const onStandardDemoClicked = (staticError) => {


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:50Z

pkg/playground/react-patterns.js

    const dialogProps = {
        title: "This shouldn't be seen",
        body: React.createElement(PatternDialogBody, { clickNested: onStandardDemoClicked }),
+        static_error: staticError,


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:50Z

pkg/shell/credentials.jsx

@@ -316,14 +316,12 @@ const UnlockKey = ({ keyName, load, onClose }) => {
                       <Button variant='link' onClick={onClose}>{_("Cancel")}</Button>
                   </>
               }>
-            <>
+            <Form onSubmit={e => { e.preventDefault(); return false }} isHorizontal>


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:50Z

pkg/shell/hosts_dialog.jsx

@@ -278,7 +280,10 @@ class AddMachine extends React.Component {
                       </Button>
                   </>}
            >
-                {body}
+                <Stack hasGutter>
+                    { this.props.dialogError && <ModalError dialogError={this.props.dialogError} />}


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:51Z

pkg/shell/superuser.jsx

-                <p>{_("Your browser will remember your access level across sessions.")}</p>
-            </>
+            <Stack hasGutter>
+                {error && <ModalError dialogError={error} />}


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:51Z

pkg/systemd/overview-cards/cryptoPolicies.jsx

@@ -183,6 +182,7 @@ const CryptoPolicyDialog = ({
                   </>
               }
        >
+            {error && <ModalError dialogError={typeof error == 'string' ? error : error.message} />}


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:51Z

pkg/systemd/overview-cards/motdCard.jsx

+                <ModalError dialogError={error}
+                             dialogErrorDetail={errorDetail} />}


These 2 added lines are not executed by any test. Details

cockpituous · 2022-12-03T10:28:51Z

pkg/systemd/services/service-details.jsx

-            {dialogError && <ModalError dialogError={_("Timer deletion failed")} dialogErrorDetail={dialogError} />}
-            {reason}
+            <Stack hasGutter>
+                {dialogError && <ModalError dialogError={_("Timer deletion failed")} dialogErrorDetail={dialogError} />}


This added line is not executed by any test. Details

cockpituous · 2022-12-03T10:28:52Z

pkg/tuned/dialog.jsx

@@ -314,6 +313,7 @@ const TunedDialog = ({
                   </>
               }
        >
+            {error && <ModalError dialogError={typeof error == 'string' ? error : error.message} />}


This added line is not executed by any test. Details

martinpitt added the blocked label Apr 5, 2022

martinpitt requested a review from garrett April 5, 2022 09:59

martinpitt temporarily deployed to cockpit-dist April 5, 2022 10:01 Inactive

martinpitt mentioned this pull request Apr 5, 2022

kdump: Show current journal when (re)starting service fails #17214

Merged

garrett mentioned this pull request Apr 7, 2022

Redesign delete/remove dialog cockpit-project/cockpit-machines#652

Merged

KKoukiou removed the blocked label Nov 30, 2022

KKoukiou self-assigned this Nov 30, 2022

KKoukiou force-pushed the dialog branch from 274b1d9 to e5e84f9 Compare November 30, 2022 12:00

KKoukiou temporarily deployed to cockpit-dist November 30, 2022 12:06 Inactive

KKoukiou force-pushed the dialog branch from e5e84f9 to 1924f95 Compare November 30, 2022 12:18

KKoukiou marked this pull request as ready for review November 30, 2022 12:18

KKoukiou temporarily deployed to cockpit-dist November 30, 2022 12:23 Inactive

KKoukiou force-pushed the dialog branch from 1924f95 to 195b73d Compare November 30, 2022 13:59

KKoukiou temporarily deployed to cockpit-dist November 30, 2022 14:04 Inactive

KKoukiou force-pushed the dialog branch from 195b73d to 3264b02 Compare November 30, 2022 15:12

KKoukiou temporarily deployed to cockpit-dist November 30, 2022 15:18 Inactive

KKoukiou force-pushed the dialog branch from 3264b02 to 50f39ab Compare November 30, 2022 17:36

KKoukiou temporarily deployed to cockpit-dist November 30, 2022 17:41 Inactive

KKoukiou force-pushed the dialog branch from 50f39ab to 6d301c1 Compare December 1, 2022 10:27

KKoukiou temporarily deployed to cockpit-dist December 1, 2022 10:33 Inactive

KKoukiou previously approved these changes Dec 1, 2022

View reviewed changes

martinpitt added 2 commits December 1, 2022 18:23

lib: Move Dialog static_error before the body

825dae9

KKoukiou dismissed their stale review via 2961fbc December 1, 2022 17:25

KKoukiou force-pushed the dialog branch from 6d301c1 to 2961fbc Compare December 1, 2022 17:25

KKoukiou temporarily deployed to cockpit-dist December 1, 2022 17:31 Inactive

KKoukiou marked this pull request as draft December 1, 2022 17:41

KKoukiou force-pushed the dialog branch from 2961fbc to 7e4820b Compare December 1, 2022 17:42

lib: Put Dialog errors at the top

cc8cb6b

See https://www.patternfly.org/v4/components/form/design-guidelines/#error-validation-on-submission

KKoukiou force-pushed the dialog branch from 7e4820b to 4dacda2 Compare December 1, 2022 17:46

KKoukiou temporarily deployed to cockpit-dist December 1, 2022 17:51 Inactive

KKoukiou force-pushed the dialog branch from 4dacda2 to 351895d Compare December 1, 2022 18:49

KKoukiou marked this pull request as ready for review December 1, 2022 18:52

KKoukiou temporarily deployed to cockpit-dist December 1, 2022 18:56 Inactive

all: Show ModalError on top of dialogs

c940100

See https://www.patternfly.org/v4/components/form/design-guidelines/#error-validation-on-submission

KKoukiou force-pushed the dialog branch from 351895d to c940100 Compare December 2, 2022 00:12

KKoukiou temporarily deployed to cockpit-dist December 2, 2022 00:17 Inactive

jelly approved these changes Dec 2, 2022

View reviewed changes

cockpituous reviewed Dec 3, 2022

View reviewed changes

KKoukiou merged commit 4a7a40f into cockpit-project:main Dec 3, 2022

jelly mentioned this pull request Mar 2, 2023

Show dialog errors on top cockpit-project/cockpit-podman#1221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show dialog errors at the top #17221

Show dialog errors at the top #17221

martinpitt commented Apr 5, 2022 •

edited by KKoukiou

martinpitt commented Apr 5, 2022

jelly commented Apr 5, 2022

martinpitt commented Apr 5, 2022

jelly commented Apr 5, 2022

martinpitt commented Apr 5, 2022

garrett commented Apr 6, 2022

garrett commented Apr 7, 2022

jelly commented Apr 8, 2022

garrett commented Apr 21, 2022 •

edited

martinpitt commented Apr 21, 2022

garrett commented Apr 25, 2022

garrett commented Nov 7, 2022

KKoukiou commented Nov 30, 2022

martinpitt commented Nov 30, 2022

KKoukiou commented Nov 30, 2022

KKoukiou left a comment

martinpitt commented Dec 1, 2022

garrett commented Dec 1, 2022

martinpitt commented Dec 2, 2022

jelly commented Dec 2, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

cockpituous Dec 3, 2022

		<ModalError dialogError={error}
		dialogErrorDetail={errorDetail} />}

Show dialog errors at the top #17221

Show dialog errors at the top #17221

Conversation

martinpitt commented Apr 5, 2022 • edited by KKoukiou

martinpitt commented Apr 5, 2022

jelly commented Apr 5, 2022

martinpitt commented Apr 5, 2022

jelly commented Apr 5, 2022

martinpitt commented Apr 5, 2022

garrett commented Apr 6, 2022

garrett commented Apr 7, 2022

jelly commented Apr 8, 2022

garrett commented Apr 21, 2022 • edited

martinpitt commented Apr 21, 2022

garrett commented Apr 25, 2022

garrett commented Nov 7, 2022

KKoukiou commented Nov 30, 2022

martinpitt commented Nov 30, 2022

KKoukiou commented Nov 30, 2022

KKoukiou left a comment

Choose a reason for hiding this comment

martinpitt commented Dec 1, 2022

garrett commented Dec 1, 2022

martinpitt commented Dec 2, 2022

jelly commented Dec 2, 2022

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

Choose a reason for hiding this comment

martinpitt commented Apr 5, 2022 •

edited by KKoukiou

garrett commented Apr 21, 2022 •

edited