-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Loading indicator for panels 2 #441
Conversation
Deployed to https://pr-441-aescan.stg.aepps.com |
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.
For now, it works pretty decently but it involves slight code duplication that we can easily reduce by creating a composable. In the future we could probably add some animation to make it even nicer but I don't think we should focus on that right now
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.
It looks really good to me. The only thing I noticed is that the vertical padding in the panel looks less then the one applied with tables so I would keep the same one but I approve in advance.
Good catch, adjusted |
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.
I suggested one minor improvement for the sake of consistency and I think it would be also nice to somehow preload or at least cache the loading indicator svg so it's visible in the very beginning just like the loading text.
That being said these are minor stuff that we can also fix with a follow-up task if you want.
2023-08-10.08-43-06.mp4
@janmichek please ignore the part about caching... I had "disable caching" enabled 🤦♂️ |
LGTM |
Description
fixes #391
Demo
firefox_8Fz0RHlF1N.mp4
Checklist:
I have read and followed the Contributing Guide
add panel loader