-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
shell: Animate host switcher caret #19815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your first contribution, exciting! This still needs some work though, I left some comments below.
@garrett I'm afraid this isn't really my area of expertise. Could you please have a look? The change looks technically plausible now. Thanks @MeastroZI ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
We already had caret animation like this in other parts of Cockpit (expandable tables, for example). Thanks for adding it to the dropdown of the host switcher too!
Thanks @MeastroZI ! I rebased your branch, fixed the commit message and the trailing space in the CSS, and drive this to completion. I also tested it and it works nicely now. Cheers! 🎉 |
This pixel test failure points out a problem, the caret isn't yet animated to close again. Most likely we'll just need to wait a bit, checking. |
Ah, it is wrong because the caret is really the wrong way around -- it should point down when closed, and point up when opened. I flipped it around. The new caret is also a bit bigger, which is fine. updating the pixel tests accordingly. |
"Introducing a minor update:
Changes: Improved caret transition in the document, accessible through .
View the modifications :
Screencast.2024-01-05.19.00.46.mp4
Previous version :
Screencast.2024-01-05.18.44.52.mp4