cockpit-project / cockpit

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

login: Split alert into title/description #21130

Closed martinpitt closed 1 week ago

martinpitt commented 2 weeks ago

This makes it conformant to the current PF design.

Bring the DOM structure closer to current PF5's alert, and adjust/simplify CSS accordingly. Thanks Garrett for working this out!

Also tighten the error message checks in the tests, in particular for check-ws-bastion.

Rework the "no-cockpit" message: Telling the user which OSes are supported is nice at some level, but doesn't help operationally when they want to connect to that server. Thus, suggest to install cockpit-system, which is the recommended way out of this error.


Reproducer for showing the banner on login page (do this in a test VM):

printf '[Session]\nBanner = /tmp/banner\n' | sudo tee -a /etc/cockpit/cockpit.conf
printf 'Hello from my laptop\nsecond line\n' > /tmp/banner
sudo systemctl stop cockpit
# so that cockpit can read /tmp/banner
sudo setenforce 0

Banner:

image

Login alert without message (title only):

image

Login alert with message (unknown server, or wrong password):

image

Redesigned "no cockpit" error when in beiboot mode:

Screen Shot 2024-10-24 at 08 50 13

martinpitt commented 2 weeks ago

@garrett 's initial design:

design

martinpitt commented 1 week ago

Next round. I incorporated @garrett 's design/demo, and adjusted it a bit (actual PF colors and 2px instead of 3px top border width). The login failure alerts look quite fine now:

image

image

However, the icon is now centered with the description (or title) if it has multiple lines. Easily seen with setting

printf '[Session]\nBanner = /tmp/banner\n' | sudo tee -a /etc/cockpit/cockpit.conf
printf 'Hello from my laptop\nsecond line\n' > /tmp/banner
sudo systemctl stop cockpit
# so that cockpit can read /tmp/banner
sudo setenforce 0

On main:

image

This PR:

image

This also affects the changed host key alert in the same way.

I'll look at that next. But I wanted to do a full round of tests in between, as the previous round still had a failure on arch, and I changed some DOM structure.

garrett commented 1 week ago

However, the icon is now centered with the description (or title) if it has multiple lines. Easily seen with setting

Whoops! My fault. I think the vertical spacing is incorrect too. But these things should be easy to fix.

Reference screenshot from https://www.patternfly.org/components/alert/html/#inline-variations (yeah, it's success state instead of error, but it's inline with a description):

image

And for the long title, from https://www.patternfly.org/components/alert/html/#variations:

image

(The alignment isn't right in PF5 either, but it's closer.)

garrett commented 1 week ago

Changes that should fix the icon alignment:

.pf-v5-c-alert {
  align-items: start;
}

.pf-v5-c-alert__icon {
  margin-top: calc((1cap - 1ex) / -2);
}

https://blog.kizu.dev/cap-height-align/

cap is "relatively" "new"; this says to take the difference between the capital height and adjust it accordingly, and it's / 2 for just the top part (not bottom as well) and negative to make it negative margin upward https://blog.kizu.dev/cap-height-align/ talks about the theory, but it's using vertical align we're in a grid, so I shifted it up by half the difference, which isn't mentioned here, but it's the same principle overall

martinpitt commented 1 week ago

@garrett With your original formula, it looks oddly off:

image

image

Is your intent to align them at the bottom line, not at the top?

when I turn the -2 into 2 to move the icon down instead of up (which feels a bit nicer to me), it looks like this:

image

image

garrett commented 1 week ago

OK, that's weird.

I guess something's different in the sandbox vs. the live environment. The gap is also wrong weirdly. I'll need to look more.

The basic approach still looks fine, but the details need some adapting/tuning apparently. (The problem of in vitro vs. in vivo. My CSS was working fine in the "petri dish", but clearly not here in context.)

martinpitt commented 1 week ago

@garrett I pushed your suggestion for now (plus test adjustments). The red login errors are straightforward to reproduce. See https://github.com/cockpit-project/cockpit/pull/21130#issuecomment-2426085108 if you also want to see the blue banner (do all this in a test VM preferably)

martinpitt commented 1 week ago

@garrett I pushed what I have, and put current screenshots into the description. I resolved/hid most comments here to make it less noisy. I think the alignment is still not quite perfect, but at least not completely wrong. I'd also like to confirm/discuss the verbiage around "no-cockpit" above. Thanks!

martinpitt commented 1 week ago

Thanks @garrett for your help and the iterations! I track the remaining bits in https://issues.redhat.com/browse/COCKPIT-1178

The code review should hopefully be straightforward.