cockpit-project / cockpit

Cockpit is a web-based graphical interface for servers.
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
10.68k stars 1.07k forks source link

confusing date/time form helper in reboot dialog #20669

Open martinpitt opened 3 weeks ago

martinpitt commented 3 weeks ago
          Garrett │ pitti: looks mostly fine. Invalid date format warning should be under the date, not time.

This isn't just a bug, more of a mis-design. If both are invalid, we show it like this:

image

Presumably to avoid the strings becoming too long and not fitting next to each other?

So let's do this as a follow-up.

Originally posted by @martinpitt in https://github.com/cockpit-project/cockpit/issues/20668#issuecomment-2192189042

martinpitt commented 3 weeks ago

@garrett How about we shorten the string to "invalid" and handle time and date separately? Then the string is hopefully short enough (translations!) to fit into the field width.

garrett commented 1 week ago

PatternFly has the (!) and error states for inputs when things are invalid:

https://www.patternfly.org/components/forms/form#invalid

image

Can we set the states for the invalid forms properly?

Also, the elements are overlapping each other for some odd reason. Crop to highlight:

image

garrett commented 1 week ago

Specifically, the date and time should look like this:

image

image

I'm not sure why the (!) is on opposite sides. I guess the calendar is a button and the clock is an icon. And the text is about invalid versus asking for a valid time. It's weird and inconsistent.

We could have 3 strings (which could all be translated):

And then have the corresponding field(s) in error mode.

And date should always be preselected as current date, right? (It doesn't look like this in the screenshot — but why not?) So the errors will mainly be the time, not date. I guess we could even prefill the time as 5 minutes (assuming someone would want at least a few minutes if they're not picking immediate) in the future as a default, which could be changed.

garrett commented 1 week ago

Mockups. First is the label on the left size (without wrapping), but this is a little too wide. We might actually want to use stacked labels (which is the PF default, but we usually do not use... although switching it up in some places and considering it for the future where we can might not be a bad idea).

image

The other things that are in common between the two layouts:

I just realized I didn't add the timezone. The most obvious thing to do would be to show the Cockpit system's timezone right after the time.

Additionally, perhaps the helper text for "computer will reboot in X minutes" would be for the modal, instead of below the time?

If so, it'd look like this:

image

(Error states in the designs above would still apply.)

garrett commented 1 week ago

OK, here's one where I backported the changes from the default into the merged set of mockups:

image

Anyway, I think this should be handled in multiple PRs; I don't expect it to be handled all at once. (This is part of the reason why I'm suggesting the stacked version and have the side version mocked up still.)

garrett commented 1 week ago

For a point of reference, I think it's good to have a short string that's right under the error. Here's what it looks like without being aligned (either to the group datetime group or the whole group):

image

If anything, the second of these would be preferable, I think... but the real ideal one would be directly under the input.

But if we do default to a suggested date and time, then that removes the chance that someone will end up in an error state.